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

Create CLI commands for AWS auth manager to create Amazon Verified Permissions related resources #36799

Merged
merged 5 commits into from
Jan 15, 2024

Conversation

vincbeck
Copy link
Contributor

AWS auth manager uses Amazon Verified Permissions service as authorizer. This PR creates two CLI commands in order to help creating the resources needed for the AWS auth manager to work:

  • init_avp. Initialize the resources needed in Amazon Verified Permissions
  • update_avp_schema. Update the Amazon Verified Permissions policy store schema to the latest version

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.

@vincbeck vincbeck requested a review from potiuk as a code owner January 15, 2024 20:58
Copy link
Contributor

@o-nikolas o-nikolas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code lgtm!

@vincbeck vincbeck merged commit 81be6ac into apache:main Jan 15, 2024
53 of 54 checks passed
@vincbeck vincbeck deleted the vincbeck/aws-am-avp-cli branch January 15, 2024 22:59
@potiuk
Copy link
Member

potiuk commented Jan 16, 2024

For whatever reason this one fails Airflow compatibility checks in canary build. I will take a look why it did not fail one here https://github.com/apache/airflow/actions/runs/7539705871/job/20523381688

@potiuk
Copy link
Member

potiuk commented Jan 16, 2024

But it would be great if you could take a look @vincbeck @o-nikolas - maybe it will be obvious to you

@o-nikolas
Copy link
Contributor

But it would be great if you could take a look @vincbeck @o-nikolas - maybe it will be obvious to you

It's a very weird import error. That module certainly exists at that path and has existed for some time now. Very odd...

@vincbeck
Copy link
Contributor Author

Following up on this one, is it resolved or we need to do something about it? @Taragolis

@potiuk
Copy link
Member

potiuk commented Jan 16, 2024

#36824 should fix it.

@potiuk
Copy link
Member

potiuk commented Jan 16, 2024

It's a very weird import error. That module certainly exists at that path and has existed for some time now. Very odd...

Nope. It's been added in 2.7.0.

@potiuk
Copy link
Member

potiuk commented Jan 16, 2024

And I even know why it did not fail in PR but started to fail only in SOME PRs and main, "canary" build.

The problem is that in PR you do not run "full checks" - only a subset of the tests. And we only build and install modified providers to save time. The problem in this case is that when we do not build all providers - only few - this check only INSTALLS providers but does not VERIFY them - because verification might fail if we only install few providers, because of some dynamic cross-importing between providers.

That's why this check only fails in those PRs that - for whatever reason - build and install and VERIFY all providers.

However, I think this speed-up is not needed any more. It gave a lot savings when building providers took way more time - and I optimized it a lot since (like 10x) and we can easily build all providers now for all PRs as it takes less than 1 minute now to build them all (it used to takes 12 minutes or so).

@potiuk
Copy link
Member

potiuk commented Jan 16, 2024

Fix for future similar cases is coming.

potiuk added a commit to potiuk/airflow that referenced this pull request Jan 16, 2024
When building wheel providers took a lot of time (12 minutes) there
was an optimisation implemented to only build the affected providers
and in this case we could not run verification, because having only
subset of providers would generate errors during imports.

However, changes to make our package bulds reproducible with flit apache#35693
also improve building time for provider packages (all of them are
built under 1 minute) and .whl installation had always been rather
quick - so we can remove the optimisation now, because side effect
of it that in some cases (like apache#36799) it caused the backwards
compatibility check succeed - and subsequently continue failing in
main canary build.
potiuk added a commit that referenced this pull request Jan 16, 2024
…36825)

When building wheel providers took a lot of time (12 minutes) there
was an optimisation implemented to only build the affected providers
and in this case we could not run verification, because having only
subset of providers would generate errors during imports.

However, changes to make our package bulds reproducible with flit #35693
also improve building time for provider packages (all of them are
built under 1 minute) and .whl installation had always been rather
quick - so we can remove the optimisation now, because side effect
of it that in some cases (like #36799) it caused the backwards
compatibility check succeed - and subsequently continue failing in
main canary build.
potiuk added a commit that referenced this pull request Feb 7, 2024
…36825)

When building wheel providers took a lot of time (12 minutes) there
was an optimisation implemented to only build the affected providers
and in this case we could not run verification, because having only
subset of providers would generate errors during imports.

However, changes to make our package bulds reproducible with flit #35693
also improve building time for provider packages (all of them are
built under 1 minute) and .whl installation had always been rather
quick - so we can remove the optimisation now, because side effect
of it that in some cases (like #36799) it caused the backwards
compatibility check succeed - and subsequently continue failing in
main canary build.

(cherry picked from commit 7086192)
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Feb 19, 2024
ephraimbuddy pushed a commit that referenced this pull request Feb 22, 2024
…36825)

When building wheel providers took a lot of time (12 minutes) there
was an optimisation implemented to only build the affected providers
and in this case we could not run verification, because having only
subset of providers would generate errors during imports.

However, changes to make our package bulds reproducible with flit #35693
also improve building time for provider packages (all of them are
built under 1 minute) and .whl installation had always been rather
quick - so we can remove the optimisation now, because side effect
of it that in some cases (like #36799) it caused the backwards
compatibility check succeed - and subsequently continue failing in
main canary build.

(cherry picked from commit 7086192)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) kind:documentation provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants