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

Move up xpkg build command to crossplane #4694

Merged
merged 25 commits into from Oct 3, 2023
Merged

Conversation

lsviben
Copy link
Contributor

@lsviben lsviben commented Sep 27, 2023

Description of your changes

Migrates the up xpkg build command to Crossplane as part of the devEx improvements.

Some notes about the PR for the reviewers:
Most of the PR is just a copy of the https://github.com/upbound/up/internal/pkg/xpkg to internal/xpkg/v2 (with some unused files removed, non build related), and the copy is in the first commit. The rest of the commits are adjusting the code to Crossplane linting, adding functions to the build, and trying to make it more Crossplane then Upbound (mostly in comments).

I skipped for now the push command as it has a lot of Upbound specific code, like authenticating and creating repositories, so I felt that that deserved a separate, more readable PR, where some of this code would be made less Upbound-specific.

Other commands like dep init will be added separately.

Also I skipped the alpha batch and xpextract commands as they are alpha. We could add those later on.

How this interacts with the existing commands and code in Crossplane

As we already have a build/push commands in crossplane(and a couple more), and there is very similar code in internal/xpkg initially I tried to merge the code from up to that (while keeping the commands separate). And in most cases its ok as it seems that both share the same roots. But it got complicated fast because up had a lot of changes on the original code, and the Crossplane code had also its changes, so it was a nightmare to merge.

Thats why the up xpkg code was moved into v2, as it is anyway planned in the future to replace the current Crossplane commands, due to having a richer feature set. So there will be duplicated code in v2, but its much simpler that way, and we can remove v1 at one point.

Which command is added

build:
Improved version of the current Crossplane build which allows for packing the manifest togather with the controller/function image and examples.

Further improvements

There is a bunch of stuff that can still be done here but I wanted to keep this already big PR simpler:

  • deprecating the current build command
  • deduplication of code between v1 and v2 internal/xpkg (partially done)
  • adding a pretty logger like the one up uses
  • adding other commands in up xpkg

Fixes #4657

I have:

How was this tested

Just ran the command for various packages:

Commands:
  build configuration
    Build a Configuration package.

  build provider
    Build a Provider package.

  build function
    Build a Function package.

  install configuration <package> [<name>]
    Install a Configuration package.

  install provider <package> [<name>]
    Install a Provider package.

  update configuration <name> <tag>
    Update a Configuration package.

  update provider <name> <tag>
    Update a Provider package.

  push configuration <tag>
    Push a Configuration package.

  push provider <tag>
    Push a Provider package.

  xpkg build
    Build a package, by default from the current directory.

Run "kubectl crossplane <command> --help" for more information on a command.
~/go/test/function-dummy (main) $ kubectl crossplane xpkg build -f package/ --controller=provider-dummy:0.1.0
xpkg saved to /home/lovro/go/test/function-dummy/package/function-dummy-303029fb9515.xpkg

@lsviben lsviben requested a review from phisco September 27, 2023 14:31
@lsviben lsviben marked this pull request as ready for review September 27, 2023 14:50
@lsviben lsviben requested review from negz, a team and turkenh as code owners September 27, 2023 14:50
@lsviben
Copy link
Contributor Author

lsviben commented Sep 27, 2023

codecov/project Failing after 1s — 65.51% (-4.26%) compared to 89b5fe5

Looks like the code coverage is failing as the copied over code is not that well tested. I try to add some tests there

Copy link
Member

@tnthornton tnthornton left a comment

Choose a reason for hiding this comment

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

I'm really excited to see this effort getting kicked off. Given my context for these changes over in upbound/up, I mostly targeted the following:

  • Did we bring over more than we needed to (for example dep related functionality and that ties into xpls)?
  • Do we want to make some of the same assumptions here that were made in upbound/up?

My comments below are all related to that. Let me know if you have any questions or if there is other context I can provide 👍

cmd/crank/xpkg/dep.go Outdated Show resolved Hide resolved
internal/xpkg/v2/parser/linter/linter.go Outdated Show resolved Hide resolved
internal/xpkg/v2/testdata/config_package.yaml Outdated Show resolved Hide resolved
@lsviben
Copy link
Contributor Author

lsviben commented Sep 27, 2023

I'm really excited to see this effort getting kicked off. Given my context for these changes over in upbound/up, I mostly targeted the following:

  • Did we bring over more than we needed to (for example dep related functionality and that ties into xpls)?
  • Do we want to make some of the same assumptions here that were made in upbound/up?

My comments below are all related to that. Let me know if you have any questions or if there is other context I can provide 👍

Thanks for taking the time to review @tnthornton !

I initially started with adding all the commands, but then removed some because they are alpha (batch and xpextract), then push as I think we need to make it a bit less Upbound-centric. So I was left with these 3.

Looking at it now again, and after the discussion on the SIG meeting, where we confirmed that the most important are build and push I feel like its a good idea to drop init and dep from this PR to make it simpler. Especially dep which has a lot of code, and is only usefull when we add xpls. So ill do that

@lsviben
Copy link
Contributor Author

lsviben commented Sep 28, 2023

Ok, made the following changes:

  • Removed dep, init and related code from
  • Removed pterm
  • Removed alpha gate
  • Bumpred crossplane runtime to include the Lintable interface, which we need for deduplicating the linter stuff. And deduplicated it.

@lsviben lsviben changed the title Move up xpkg commands to crossplane (build, dep, init) Move up xpkg build command to crossplane Sep 28, 2023
internal/xpkg/v2/doc.go Outdated Show resolved Hide resolved
internal/xpkg/v2/writer.go Outdated Show resolved Hide resolved
internal/input/prompt.go Outdated Show resolved Hide resolved
internal/xpkg/v2/scheme/scheme.go Outdated Show resolved Hide resolved
internal/xpkg/v2/name.go Outdated Show resolved Hide resolved
cmd/crank/xpkg/build.go Outdated Show resolved Hide resolved
internal/xpkg/v2/layers.go Outdated Show resolved Hide resolved
internal/xpkg/v2/build.go Outdated Show resolved Hide resolved
cmd/crank/xpkg/build.go Outdated Show resolved Hide resolved
@phisco
Copy link
Contributor

phisco commented Sep 28, 2023

Checked the output of this against the one from up and it's exactly the same as expected:
https://explore.ggcr.dev/?image=docker.io/phisco/test:v0.0.0-provider-nop-with-up
https://explore.ggcr.dev/?image=docker.io/phisco/test:v0.0.0-provider-nop-with-crank

We can definitely refactor it further, but as an starting point and initial porting of the up functionalities, I'd say it's ok 👍

Copy link
Contributor

@haarchri haarchri left a comment

Choose a reason for hiding this comment

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

LGTM - thanks

test successful done!
./crank xpkg build --ignore="init/*.yaml,kind-config.yaml" --output=test.xpkg

errParseAuth = "an auth extension was supplied but could not be parsed"
errAuthNotAnnotated = "an auth extension was supplied but but the " + providerConfigKind + " object could not be found"
errNotExactlyOneMeta = "package must contain exactly one meta object"
authMetaAnno = "auth.upbound.io/group"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this something we need here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

given this is used to configure ProviderConfig functionality in console.upbound.io, probably not. It does imply that any provider in the future that wants to take advantage of that functionality would have to be built with up, which is the case today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the auth extension option then. We can leave this to be up specific

Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
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.

move the xpkg commands from up to upstream
5 participants