Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: make fetchEmails async #704

Closed
wants to merge 1 commit into from
Closed

feat: make fetchEmails async #704

wants to merge 1 commit into from

Conversation

donno2048
Copy link

@donno2048
Copy link
Author

WIP, hence PR title isn't linted please approve for testing workflow to run

@chuang8511
Copy link
Contributor

Hi @donno2048 ,
Thanks for your time. Could you help me do a few things?

  1. Change the title when the PR is ready to be reviewed. We will start to confirm it when all CI passed.
  2. Provide the sample recipe for us that we can easily QA in our sides.

Please feel free to ask us any question!
Thank you again! 🙇

@donno2048
Copy link
Author

Sure, I'll change the title when the PR is ready, as for QA, isn't the workflow already doing it?

@chuang8511
Copy link
Contributor

Hi @donno2048 ,
Thanks for your question!
The current workflow only QA high level API. It does not test the implementation you do.
In the future, we will have the recipe automation function to make this part efficient.

Before that, we need to ensure the functionality by end to end test through the developers.

@donno2048
Copy link
Author

My change doesn't affect the function as a whole, only changes the way it is evaluated, so I won't make any new tests as there's nothing new to test...

@chuang8511
Copy link
Contributor

I don't mean you have to add a new test case in the codebase.
I mean when we modify the code, we have to do end-to-end test by developers.

For example, in this PR, I will do manual test by this recipe to ensure it works as expected.

version: v1beta
component:
    email-0:
        type: email
        task: TASK_READ_EMAILS
        input:
            search:
                limit: 5
                mailbox: Inbox
        setup:
            email-address: ${variable.your_gmail_address}
            password: ${secret.instill-email-component}
            server-address: imap.gmail.com
            server-port: 993
variable:
    your_gmail_address:
        title: your-gmail-address
        instill-format: string
output:
    result:
        title: result
        value: ${email-0.output.emails}

Because we already have had this recipe, please help us modify the title.
We will do QA by this recipe when we review it.

Thanks for your contribution! 🙇

@donno2048
Copy link
Author

Are you just asking me to change the title? If you already have a recipe to test this with I don't understand what you want..?

@chuang8511
Copy link
Contributor

Please just update the title. We will QA on our own.

@donno2048 donno2048 changed the title WIP: Make fetchEmails async feat: make fetchEmails async Oct 9, 2024
@donno2048 donno2048 marked this pull request as draft October 9, 2024 07:01
@donno2048 donno2048 marked this pull request as ready for review October 9, 2024 07:02
@donno2048
Copy link
Author

Done

Comment on lines +177 to +178
if err != nil {
return nil, err
}

Check warning

Code scanning / CodeQL

Unreachable statement

This statement is unreachable.
@chuang8511
Copy link
Contributor

chuang8511 commented Oct 9, 2024

@donno2048
Please make sure your code pass the test locally first.
For component, you could run $ go test ./pkg/component/{type-of-component}/{component}/v0/...

image

@donno2048
Copy link
Author

Fixed, sorry, I'm on mobile so it's a little complicated to run tests....

@chuang8511
Copy link
Contributor

@donno2048
Thanks for your contribution.
Could you take a look about the rule of sending PR?

Let's follow it to make the commit history clean.

Make and Commit Changes: Implement your changes and commit them. We encourage you to follow these best practices for commits to ensure an efficient review process:
Adhere to the conventional commits guidelines for meaningful commit messages.
Follow the 7 rules of commit messages for well-structured and informative commits.
Rearrange commits to squash trivial changes together, if possible. Utilize git rebase for this purpose.

@donno2048 donno2048 closed this Oct 10, 2024
@donno2048 donno2048 reopened this Oct 10, 2024
@donno2048
Copy link
Author

I'll make a new PR with a clean history...

Lots of force pushes here and stuff

@donno2048 donno2048 closed this Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants