Re: [PR] Add four unit tests for aws/utils [airflow]
boring-cyborg[bot] commented on PR #38820: URL: https://github.com/apache/airflow/pull/38820#issuecomment-2057425722 Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
potiuk commented on PR #38820: URL: https://github.com/apache/airflow/pull/38820#issuecomment-2057426525 Now those tests looked more useful -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
potiuk merged PR #38820: URL: https://github.com/apache/airflow/pull/38820 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
o-nikolas commented on PR #38820: URL: https://github.com/apache/airflow/pull/38820#issuecomment-2057236733 > > You will need to remove these lines from the project structure tests: > > https://github.com/apache/airflow/blob/main/tests/always/test_project_structure.py#L85-L86 > > Do you know if the others will be reviewing this PR any time soon? Also [PR 38819](https://github.com/apache/airflow/pull/38819) has been approved by one reviewer and since you're named as one of the reviewers, I was hoping you can review it when you get a chance. Thanks. It's not really about the number of approvers, it was more that @potiuk and @hussein-awala had some dissenting opinions on the PR, so I wanted to give them the opportunity to review the current version after your changes and see if they find it more acceptable now. However, they do seem to be busy! So if no one checks in by end of today or tomorrow I can merge this. We can always make changes later :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
slycyberguy commented on PR #38820: URL: https://github.com/apache/airflow/pull/38820#issuecomment-2054526341 > You will need to remove these lines from the project structure tests: > > https://github.com/apache/airflow/blob/main/tests/always/test_project_structure.py#L85-L86 Do you know if the others will be reviewing this PR any time soon? Also [PR 38819](https://github.com/apache/airflow/pull/38819) has been approved by one reviewer and since you're named as one of the reviewers, I was hoping you can review it when you get a chance. Thanks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
slycyberguy commented on PR #38820: URL: https://github.com/apache/airflow/pull/38820#issuecomment-2048617564 > You will need to remove these lines from the project structure tests: > > https://github.com/apache/airflow/blob/main/tests/always/test_project_structure.py#L85-L86 I took care of it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
o-nikolas commented on code in PR #38820: URL: https://github.com/apache/airflow/pull/38820#discussion_r1558210182 ## tests/providers/amazon/aws/utils/test_sagemaker.py: ## Review Comment: Sounds good! I approved. We'll need have all conversations resolved before merging. I'll let @potiuk and @hussein-awala reply back to this thread to see if they have any concerns. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
slycyberguy commented on code in PR #38820: URL: https://github.com/apache/airflow/pull/38820#discussion_r1558189422 ## tests/providers/amazon/aws/utils/test_sagemaker.py: ## Review Comment: No problem. Thanks for the suggestions. I renamed them similar to your examples. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
o-nikolas commented on code in PR #38820: URL: https://github.com/apache/airflow/pull/38820#discussion_r1558170657 ## tests/providers/amazon/aws/utils/test_sagemaker.py: ## Review Comment: I think maybe `response_dict_json` would be more accurate since we're parsing it as json. Or maybe `response_nested_dict` if we really want to keep it dict. But any who, naming aside. Thanks for adding the tests :slightly_smiling_face: and sorry you got some suspicious push back from us! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
slycyberguy commented on code in PR #38820: URL: https://github.com/apache/airflow/pull/38820#discussion_r1558165988 ## tests/providers/amazon/aws/utils/test_sagemaker.py: ## Review Comment: It's not generated. I tried to come up with a more descriptive name for the message samples. One was a list of dict and the other was a list of dict of dict. That's why I chose those names. Anyway, I just renamed the fixture labels to sample_message_1 and sample_message_2. Hope that works. Let me know your suggestions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
o-nikolas commented on code in PR #38820: URL: https://github.com/apache/airflow/pull/38820#discussion_r155807 ## tests/providers/amazon/aws/utils/test_sagemaker.py: ## Review Comment: I didn't review the previous tests that have been removed from this PR, but the tests remaining seem to add coverage. Hard to tell for sure if they were generated (there are some weird names like `response_dict_dict` that seem unnatural). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
slycyberguy commented on code in PR #38820: URL: https://github.com/apache/airflow/pull/38820#discussion_r1556846390 ## tests/providers/amazon/aws/utils/test_sagemaker.py: ## Review Comment: I removed those tests. I figured they were necessary because those services were listed in issue #35442 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
potiuk commented on code in PR #38820: URL: https://github.com/apache/airflow/pull/38820#discussion_r1555891937 ## tests/providers/amazon/aws/utils/test_sagemaker.py: ## Review Comment: I had the same impression that most of the tests produced / generated here are useless. I think it's not a good idea to clutter and make more of those tests that are really not testing anything - that's why in the other PR I've asked whether the tests were generated. I think all those PRs where the tests were generated to start with and then replicated should not make it into our codebase. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
potiuk commented on code in PR #38820: URL: https://github.com/apache/airflow/pull/38820#discussion_r1555893343 ## tests/providers/amazon/aws/utils/test_sagemaker.py: ## Review Comment: Potential Value added vs. time needed to review/classify those tests do not seem to be worth it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
hussein-awala commented on code in PR #38820: URL: https://github.com/apache/airflow/pull/38820#discussion_r129961 ## tests/providers/amazon/aws/utils/test_rds.py: ## Review Comment: This test is useless ## tests/providers/amazon/aws/utils/test_sagemaker.py: ## Review Comment: same here -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] Add four unit tests for aws/utils [airflow]
slycyberguy opened a new pull request, #38820:
URL: https://github.com/apache/airflow/pull/38820
Unit tests for:
airflow/provider/amazon/aws/utils/sagemaker.py
airflow/provider/amazon/aws/utils/rds.py
airflow/provider/amazon/aws/utils/sqs.py
airflow/provider/amazon/aws/utils/tags.py
---
**^ Add meaningful description above**
Read the **[Pull Request
Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)**
for more information.
In case of fundamental code changes, an Airflow Improvement Proposal
([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals))
is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party
License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a
newsfragment file, named `{pr_number}.significant.rst` or
`{issue_number}.significant.rst`, in
[newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
