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

Break uptest & crddiff into two separate binaries in preparation of moving uptest under to the Crossplane organization #183

Merged
merged 4 commits into from
Feb 28, 2024

Conversation

ulucinar
Copy link
Contributor

@ulucinar ulucinar commented Feb 27, 2024

Description of your changes

Currently, the uptest tool and the crddiff tool are implemented as the two commands of a single application, although they have completely different purposes. This was done due to a gap in the build pipelines of this repo. As we would like to move the uptest tool under to the Crossplane organization, it's time to separate them :)

This PR also introduces the Publish Artifacts to GitHub & Promote Artifacts in S3 steps in the publish-artifacts CI job to publish & promote the binary artifacts uptest, crddiff, updoc, ttr, perf and lint-provider-family in the associated S3 bucket.

Breaking Changes:

  • The uptest crddiff revision and uptest crddiff self commands are moved to the crddiff application of their own and will be available as crddiff revision & crddiff self commands. uptest e2e command stays the same.

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

The CI steps newly introduced have been tested by exporting the corresponding environment variables:

  • AWS_ACCESS_KEY_ID
  • AWS_SECRET_ACCESS_KEY
  • AWS_DEFAULT_REGION
    and running the publish and a subsequent promote make target.

Also ran the built uptest and crddiff binaries from this PR to check the available sub-commands.

Post merge:

image

moving uptest under to the Crossplane organization

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar ulucinar added the breaking-change A breaking change is introduced with this PR label Feb 27, 2024
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 @ulucinar, left a few comments

)

var (
app = kingpin.New("uptest", "Automated Test Tool for Upbound Official Providers").DefaultEnvars()
// e2e command
// e2e command (single command is preserved for backward compatibility)
// and we may have further commands in the future.
e2e = app.Command("e2e", "Run e2e tests for manifests by applying them to a control plane and waiting until a given condition is met.")
Copy link
Member

Choose a reason for hiding this comment

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

nit: It seems that after removing the crddiff subcommands from uptest, we have only the e2e subcommand. What do you think about also removing this subcommand? And using the tool without any subcommand? As I remember correctly, before introducing the crddiff related subcommands, we do not have any subcommands in uptest.

I realize this is a breaking change, but before moving it, we can think in terms of simplification. The question that came to my mind is whether we will need different subcommands in the future.

I'm just writing this as a discussion point; it's not a big deal :)

Copy link
Contributor Author

@ulucinar ulucinar Feb 28, 2024

Choose a reason for hiding this comment

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

Hi @sergenyalcin,
Thanks for the discussion, very much appreciated.
I've also considered removing the sub-command and decided to keep it with the reasons given in the comment:

  • Backwards compatibility: In addition to the official provider repositories, I think this tool is in use in other repositories we don't maintain.
  • If we implement some other commands like the preprocess command we've been discussing for a while, it may make sense to add them as sub-commands to uptest as they would be coupled with the e2e tests (preprocessing for the e2e test manifests). If we want to add them as sub-commands, then having e2e as a sub-command makes sense again. If we break it today, then we will probably want to implement such related functionality in separate applications (binaries) in the future.

Considering these two points, I've decided to keep the e2e sub-command. Btw, I think we had the e2e sub-command before crddiff. Please also see this comment.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@ulucinar ulucinar force-pushed the break-crddiff branch 2 times, most recently from 9a0db45 to be0252f Compare February 28, 2024 10:33
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
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 @ulucinar LGTM!

@ulucinar ulucinar merged commit 093bead into upbound:main Feb 28, 2024
6 checks passed
@ulucinar ulucinar deleted the break-crddiff branch February 28, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A breaking change is introduced with this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants