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

Repo restructure project - Merge helm-locker into local #74

Merged
merged 48 commits into from
Sep 25, 2024

Conversation

mallardduck
Copy link
Member

@mallardduck mallardduck commented Sep 10, 2024

This PR "merges in" the https://github.com/rancher/helm-locker project into this repo.

The code is simply dropped into pkg/helm-locker and all the other references are updated to reference the "local" module instead of the dedicated repo for it. Then the CI and related workflow scripts are all updated to account for having two CLI commands tested and compiled.


After this change the open PR for updating helm-locker deps can be reworked into this repo. At which time we will no longer need to update it in three locations before it lands. Granted it will still be needing updates here, and then subsequently in prometheus-federated repo - but two is still better than 3.


TODO

Before we merge

  • Create a method for QA to test this before merge:
    • Rework prometheus-federator to use this branch,
    • Create version of charts that use these changes; provide chart branch and directions for install (verify w/ qa)
  • Verify the proper process for QA to test these changes (i.e. does installing my prometheus-federator chart and using it to deploy rancher-project-monitoring ensure they will test helm-locker fully in my fork?) [@jbiers will try to make progress on this while I'm out]
    • Follow up with Pankaj on this process - I've already pinged him about the issue here. So may want to update that Issue with testing process info.
  • Confirm that helm-locker image isn't used standalone
    • If yes, maybe update hl-publish workflow to only publish images to ghcr.io as they are for dev/QA only?
    • Also adjust hpo-publish workflow similarly to push to ghcr.io - we know this is not user facing

O&B team feel free to work on and merge this while I'm out if you have time and are comfortable picking it up.

After we merge

  • Tag a v0.3.0 release here - which will release both images to GHCR
  • Add package/helm-project-operator label to existing issues in this repo
  • Move over the issues from helm-locker repo, label as package/helm-locker, archive helm-locker repo,
  • Implement new branching strategy (maybe after Alex PR merge, need to decide optimal order)
  • Rebuild Alex's PR to update helm-locker on this repo; will update both sets of dependencies at once.
  • Update prometheus-federator to use patched versions of this repo,
  • subsequently ensure rancher-monitoring and rancher-project-monitoring are updated and in charts

@mallardduck mallardduck requested a review from a team as a code owner September 10, 2024 17:39
@mallardduck mallardduck changed the title Repo restructure project - Merge helm-locker into local [WIP] Repo restructure project - Merge helm-locker into local Sep 11, 2024
...very temporary as a way to prevent incorrect publishing.
we will converge the files together as we have with the repos.

however we will also want to choose if helm-locker images go to ghcr or dockerhub.
worth considering ghcr instead as these are mainly test images not for end-users AFAIK. (josh to confirm that hopefully)
@mallardduck mallardduck changed the title [WIP] Repo restructure project - Merge helm-locker into local Repo restructure project - Merge helm-locker into local Sep 23, 2024

Choose a reason for hiding this comment

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

Soon we'll want to either distribute the files under pkg/helm-locker, or bring the code out into two top level helm-locker and helm-project-operator but in the interest of getting some forward motion, I'm OK with this as-is

Copy link
Member Author

Choose a reason for hiding this comment

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

To make sure I understand the context - is this to address the concern about these as go pkgs? I.e. ensuring we properly migrate from: https://pkg.go.dev/github.com/rancher/helm-locker over to an equivalent?

@@ -1,7 +1,8 @@
/.dapper
/.cache
/bin
/cmd/helm-project-operator/fs/project-operator-example.tgz.base64
!/cmd/helm-project-operator/fs/.gitkeep

Choose a reason for hiding this comment

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

why do we need to keep this dir? Can we recreated it when we need it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - that's a fair point.
I tend to favor keeping dirs like this around - feels like it keeps things uniform and gives hints to the fact that the dir will be populated as part of a successful build. Not super attached to that though if we think it should just be fully excluded.

@@ -10,7 +10,7 @@ annotations:
catalog.cattle.io/namespace: cattle-helm-system
catalog.cattle.io/permits-os: linux,windows
catalog.cattle.io/provides-gvr: helm.cattle.io.helmrelease/v1alpha1
catalog.cattle.io/rancher-version: '>= 2.6.0-0 <=2.6.99-0'

Choose a reason for hiding this comment

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

side note, at some point we might want to bring this more in line with the usual supported rancher version range, but this is super good enough for now

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering if we even bother adding it back in since we don't intend to support end-users deploying helm-locker standalone? Seems like if these are just for our internal dev/QA use then we can save ourselves having to update the annotation overtime by dropping.

Copy link
Member

@jbiers jbiers left a comment

Choose a reason for hiding this comment

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

LGTM, read though all the commits and could not see any issues. However, it would be nice to wait for one more review before merging since another set of eyes could be useful in large PRs like this one.

@mallardduck mallardduck merged commit 6309031 into main Sep 25, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants