-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🎉 Source Asana: migrate to new SAT, added base HTTP errors handling #19561
Conversation
/test connector=connectors/source-asana
Build FailedTest summary info:
|
/test connector=connectors/source-asana
Build FailedTest summary info:
|
airbyte-integrations/connectors/source-asana/unit_tests/test_streams.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-asana/source_asana/streams.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-asana/source_asana/streams.py
Outdated
Show resolved
Hide resolved
/test connector=connectors/source-asana
Build FailedTest summary info:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for migrating the config to high test strictness level.
I'm concerned about the use of an internal asana account for testing. We should create a new one for testing (ideally a paid one to benefit from all the exposed streams).
I created an issue here for this: https://github.com/airbytehq/airbyte/issues/19662
@@ -0,0 +1,84 @@ | |||
{"stream": "projects", "data": {"gid": "1201856001387054", "archived": false, "color": "dark-green", "created_at": "2022-02-22T23:20:52.238Z", "current_status": null, "custom_fields": [], "custom_field_settings": [{"gid": "1201856001387132"}, {"gid": "1201856001387112"}, {"gid": "1201856001387116"}, {"gid": "1201856001387139"}, {"gid": "1201856001387164"}], "default_view": "list", "due_on": null, "due_date": null, "followers": [{"gid": "1201617089911900"}, {"gid": "1200344886012993"}, {"gid": "1201607360212792"}], "html_notes": "<body>Standardize your RFP and evaluation process with our template to ensure you\u2019re fully prepared to receive and evaluate proposals\u2014all in the same place\u2014so you can pick the best organization for the job.</body>", "icon": "page_layout", "members": [{"gid": "1201617089911900"}, {"gid": "1200344886012993"}, {"gid": "1201607360212792"}], "modified_at": "2022-05-01T07:33:20.428Z", "name": "Speaking Engagements ", "notes": "Standardize your RFP and evaluation process with our template to ensure you\u2019re fully prepared to receive and evaluate proposals\u2014all in the same place\u2014so you can pick the best organization for the job.", "owner": {"gid": "1201617089911900"}, "permalink_url": "https://app.asana.com/0/1201856001387054/1201856001387054", "public": true, "resource_type": "project", "start_on": null, "team": {"gid": "1200344940838474"}, "workspace": {"gid": "1199152354181012"}}, "emitted_at": 1668517065006} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid this file potentially exposes sensitive internal data as the account used for testing is not a sandbox one. Please delete it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alafanechere Should we move this certification to on hold
until the dedicated sandbox account is up and running?
or should we skip checking expected records
with bypass reason? there is also a problem with two sequential reads
since we're testing the active valid account and data is not frozen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep this certification on hold
, as a dedicated test account will provide better testing guarantees. But I will let @YowanR decide, I'm not sure of the efforts required for properly seeding a sandbox account.
- name: custom_fields | ||
bypass_reason: "This stream in not available for our Test Account. 402 - Custom Fields are not available for free users or guests." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we create a dedicated paid sandbox account to cover this stream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @YowanR to make decisions here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the main issue that we are running into is that we are using an internal account instead of a sandbox account. For most connectors, we should have sandbox accounts instead of prod account. Now, a big issue here is that we actually took a very long time to get this account unblocked for OAuth. If we were to make another account, I would expect that we would block for a long time again which is detrimental to our customers. Instead, I suggest that we do two things:
- Bypass the tests that are failing due to having a prod account and not a sandbox acc (plus hide sensitive info!)
- Start setting up a sandbox account to revisit. Once we have the sandbox account, we can solve the issues that we've seen. Thoughts on this @airbytehq/connector-operations and @bazarnov ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YowanR it's unclear to me if you want this PR to certify Asana to GA.
If it's not, I'm fine with logging future work for creating a sandbox account and adding the bypass reasons. I don't want to block any feature improvement on the connector. But I'd love new GA connectors to have the testing guarantees we outlined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alafanechere I'd like us to use strict testing to get Asana to GA but, because of how hard it was to get OAuth for this account, we need to work with the restrictions of using a prod account instead of a sandbox account. Based on the comments you've left in this PR, it seems that we will need to bypass a few tests, but overall, we'll still covering most tests correct? I think that the main question I have is: do you think the strict SAT framework can work with a prod account or is that a no-no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding my 2 cents - I think that GA means that we can reliably test the connector and guarantee that everything works, and we regularly to test all the relevant parts of the source's API. To me, GA means that we can both test oAuth and have a sandbox/test account that provides data for all the streams we provide. So, I would suggest leaving the connector in beta until we get a test account with oAuth. If I had to choose, I think ensuring correct data (e.g. expected_records
) is more important than ensuring oAuth works (we value correctness more than anything else).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can split data tests and Oauth tests and set them using:
- OAuth connectivity checks and discover - prod account
- other SAT-related tests - sandbox
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea to split the difference @bazarnov !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fair! Let's do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YowanR
Sure, do we already have the sandbox so I could do it)? Or are we moving this to "on hold" until the sandbox exists?
def should_retry(self, response: requests.Response) -> bool: | ||
if response.status_code in ASANA_ERRORS_MAPPING.keys(): | ||
self.logger.error( | ||
f"Skipping stream {self.name}. {ASANA_ERRORS_MAPPING.get(response.status_code)}. Full error message: {response.text}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @erohmensing this connector is implementing a custom approach at skipping unavailable stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add it to the list 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because of how hard it was to get OAuth for this account...
Just to clarify, is enabling oAuth per-account in Asana? Do you pay for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is per account. I don't believe we had to pay extra for it, but the process itself was incredibly slow (This is main reason we had this connector move out from Q3). @igrankova and @ycherniaiev can give more context here.
@bazarnov , in the future feel free to ask for a review from all the @airbytehq/connector-operations team for the PR related to |
/test connector=connectors/source-asana
Build FailedTest summary info:
|
Can we just use the |
Just using |
/test connector=connectors/source-asana
Build PassedTest summary info:
|
@alafanechere Please review again, as you've requested changes. |
@YowanR @alafanechere @evantahler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on the SAT side. I made some wording suggestion in the bypass reasons.
airbyte-integrations/connectors/source-asana/acceptance-test-config.yml
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-asana/acceptance-test-config.yml
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-asana/acceptance-test-config.yml
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-asana/source_asana/streams.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-asana/source_asana/streams.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-asana/source_asana/streams.py
Outdated
Show resolved
Hide resolved
/publish connector=connectors/source-asana
if you have connectors that successfully published but failed definition generation, follow step 4 here |
* master: (74 commits) Fix support icon in sidebar fix: add BuildPulse report for helm ac tests (#19785) fix: yaml syntax (#19775) 🪟 🐛 Fix custom connection creation endpoint (#19702) Source facebook marketing: check "breakdowns" combinations (#19645) fix typo: notify instead of sync (#19737) find_non_rate_limited_PAT (#19736) Source Google Ads: fix schema for "campaigns" stream (#19700) 🎉 Source Asana: migrate to new SAT, added base HTTP errors handling (#19561) fix order not to randomly fail backward compatibility check (#19377) Bump helm chart version reference to 0.42.0 (#19706) fix: add extraEnv block (#19703) Bump Airbyte version from 0.40.21 to 0.40.22 (#19687) Bump helm chart version reference to 0.41.3 (#19685) Add connector builder server to airbyte proxy, kube overlays, and helm charts (#19554) dbt Cloud integration doc (#19619) 🪟 🎉 Display service token validation errors in the UI (#19578) 17644 Update Destination data type test to use the new data types (#19281) Docs: fix broken connector builder UI docs links (#19631) Bump Airbyte version from 0.40.20 to 0.40.21 (#19634) ...
What
Resolving: #19413
How
high
level of strictness402,403,404,451
unit test
🚨 User Impact 🚨
No impact is expected
Pre-merge Checklist
Expand the relevant checklist and delete the others.
Updating a connector
Community member or Airbyter
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described here