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

Add a command to push to OCI registries #362

Merged
merged 20 commits into from
Apr 21, 2022
Merged

Add a command to push to OCI registries #362

merged 20 commits into from
Apr 21, 2022

Conversation

kubasobon
Copy link
Contributor

Towards giantswarm/roadmap#391

Checklist

@kubasobon kubasobon self-assigned this Apr 19, 2022
@kubasobon kubasobon requested a review from a team April 19, 2022 07:33
Copy link
Contributor

@MarcelMue MarcelMue left a comment

Choose a reason for hiding this comment

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

Generally looking good to me - just a small question :)

for i in $(seq 1 $tries) ; do
echo "====> Attempt $i: Running: helm push ../build/*.tgz $app_catalog_name"
set +e
helm push ../build/*.tgz $app_catalog_name
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a stupid question: Where does helm actually come from here? Is it just installed by default? And should we verify that the version is recent enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

architect-orb uses architect's Dockerfile as a base image. We install a bunch of binaries in that Dockerfile, including helm. I've recently updated architect (https://github.com/giantswarm/architect/pull/728/files), and architect-orb is already using the latest release.

Copy link
Contributor

@rossf7 rossf7 left a comment

Choose a reason for hiding this comment

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

LGTM

src/commands/package-and-push-oci.yaml Outdated Show resolved Hide resolved
Co-authored-by: Ross Fairbanks <[email protected]>
@ubergesundheit
Copy link
Member

Sorry to chime in ininvited, please ignore me if my concerns don't make any sense..

Why introduce a new command? Couldn't this also work with the existing commands/jobs by specifying the catalog as oci:// url? I am asking because this seems to skip existing linting and checking steps in place with architect and app-build-suite based pushing.

@kubasobon
Copy link
Contributor Author

@ubergesundheit: Existing commands "push" to catalogs by making a Git commit in a specified repository. This command on the other hand will use helm to publish a chart to an OCI registry. That feels different enough to me to warrant a separate command. There is another command used to push with ABS enabled: https://github.com/giantswarm/architect-orb/blob/master/src/commands/package-and-push-with-abs.yaml.

I'll add an OCI + ABS version as well as an OCI registry login to this PR before re-requesting reviews.

@ubergesundheit
Copy link
Member

I'll add an OCI + ABS version as well as an OCI registry login to this PR before re-requesting reviews.

Thats good! TBH a solution where the git based pushing would be moved into a separate command and then making the pushing command/mechanism configurable in the existing jobs would be less confusing, but this is also fine for me.

@kubasobon
Copy link
Contributor Author

Thats good! TBH a solution where the git based pushing would be moved into a separate command and then making the pushing command/mechanism configurable in the existing jobs would be less confusing, but this is also fine for me.

Bundling the two sounds like a solid idea. I'll add it to the proposed PR as well.

Copy link
Contributor

@MarcelMue MarcelMue left a comment

Choose a reason for hiding this comment

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

It still makes sense to me - I struggle a bit to understand how this looks like in a project in the end, but that's a minor issue.

@kubasobon
Copy link
Contributor Author

@MarcelMue This PR adds a new command set for pushing charts to OCI registries (two new commands). It also adds two switches to the push-to-app-catalog job. push_to_appcatalog, enabled by default, runs existing commands and publishes charts to GitHub. push_to_oci_registry, disabled by default, runs new commands and publishes charts to the AzureCR OCI registry.

If we merge this and craft a release, everything will remain the same due to the defaults. If we flip the switches we can change behavior to push to OCI as well or push to it exclusively.

Do we want push_to_oci_registry right away?

@MarcelMue
Copy link
Contributor

I think it makes sense to release first with the switch disabled and then try it in select repositories. WDYT?

@kubasobon
Copy link
Contributor Author

I agree 👨‍🔬

@kubasobon
Copy link
Contributor Author

It works!
Screenshot_20220421_120317
Screenshot_20220421_120259

@kubasobon
Copy link
Contributor Author

kubasobon commented Apr 21, 2022

@ubergesundheit PTAL. I think this covers all your requests. Push to the catalogs hosted on GitHub happens by default. All one has to do to push to the OCI registries is add a push_to_oci_registry: true to .circleci/config.yml of an app.

workflows:
  version: 2
  build-and-push:
    jobs:
      - architect/push-to-app-catalog:
          context: "architect"
          executor: "app-build-suite"
          name: "package and push chart to app catalog"
          app_catalog: "giantswarm-catalog"
          app_catalog_test: "giantswarm-test-catalog"
          chart: "hello-world-app"
          # Trigger job on git tag.
          filters:
            tags:
              only: /^v.*/
          push_to_oci_registry: true

@kubasobon kubasobon merged commit bd819e6 into master Apr 21, 2022
@kubasobon kubasobon deleted the push-to-oci branch April 21, 2022 14:48
@ubergesundheit
Copy link
Member

Thanks :) Some addition to the docs would have been nice

@kubasobon
Copy link
Contributor Author

Sure, on it.

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.

4 participants