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

Update provider to no-fork architecture #46

Merged
merged 6 commits into from
Dec 11, 2024
Merged

Conversation

m1so
Copy link
Contributor

@m1so m1so commented Nov 8, 2024

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:

  • Read and followed Crossplane's contribution process.
  • Run 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

@jeanduplessis
Copy link
Contributor

@m1so Thanks for your contribution.
I chatted with the team, and we're happy to accept this PR from a technical perspective.

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.

@m1so
Copy link
Contributor Author

m1so commented Dec 9, 2024

@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 Mounts, as well as all database and rabbitmq Vault CRDs (these are however not easy to test in the current setup, as running instance of Postgres and RabbitMQ would need to be provisioned into the KinD cluster)

@jeanduplessis
Copy link
Contributor

@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.

@m1so
Copy link
Contributor Author

m1so commented Dec 10, 2024

@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

Copy link
Member

@sergenyalcin sergenyalcin left a 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.

@sergenyalcin
Copy link
Member

It seems, CI / publish-artifacts (pull_request) fails because of a deprecated action. I will bypass it and merge the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants