-
Notifications
You must be signed in to change notification settings - Fork 12
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
Update provider to no-fork architecture #46
Conversation
@m1so Thanks for your contribution. The concern is that a sufficient level of validation is needed with such a major change to ensure that things keep working. Our experience migrating providers to the no-fork architecture revealed a few issues that make us skeptical that things will keep working (although that would be nice). To proceed, we would want to see the Uptest tests pass for at least the token authentication and secret configuration resources. |
@jeanduplessis thanks for the update! I completely understand the concerns, however I'm not familiar with Upbound's CI E2E test setup. I managed to fix the E2E setup scripts locally and verified couple of resources via: make uptest UPTEST_EXAMPLE_LIST=examples/mount/mount-generic-secrets.yaml we're also running custom build of the provider on our development & staging environments, which make use of |
@m1so ok, we can also accept manual verification results. If you can share screenshots from the tests done showing a successful sync/ready state, we can accept those as well, so you don't have to struggle with Uptest. |
@jeanduplessis I can share some screenshots from k9s/Vault UI via Crossplane Slack. I also got Uptest working in c01b23e, however it doesn't run in CI so you'd have to manually verify results via make uptest UPTEST_EXAMPLE_LIST=examples/mount/mount-generic-secrets.yaml or other example resources. Alternatively let me know which tests/examples to run and I can post the outputs 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.
Thanks @m1so, we synced with @jeanduplessis on this. As he mentioned that we're happy to accept this PR from a technical perspective. Let me also thank you for taking on this complex transition for this provider.
Our only concern was about the tests. As @jeanduplessis said to me, it seems that the results you shared about test results with him were satisfactory.
So, I am approving this PR. Thanks and LGTM!
In release process pov, because two reasons I am proposing to make a major version release for this provider: v2.0.0
: There is a underlying TF provider major version bump in this change and also the main topic of PR is the no-fork change.
By releasing a major version for this provider, I think we will also provide a better UX for users.
It seems, CI / publish-artifacts (pull_request) fails because of a deprecated action. I will bypass it and merge the PR. |
Description of your changes
update the provider to use the "no-fork" architecture based on the post shared in Crossplane #sig-upjet channel
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
Tested internally in our compositions, which make use of Postgres & RabbitMQ Vault features