Update to allowing building and publishing images in hashrelease #9429
Conversation
marvin-tigera
added
release-note-required
radTuti
added
docs-not-required
marvin-tigera
removed
release-note-required
radTuti
force-pushed
the
nightly-hashrelease
branch
from
1b02d77
to
bcdd56b
Compare
radTuti
force-pushed
the
nightly-hashrelease
branch
from
bcdd56b
to
627de38
Compare
release/Makefile
Outdated
| @@ -26,8 +26,8 @@ ci: static-checks | |||
| ############################################################################### | |||
| .PHONY: hashrelease | |||
| hashrelease: bin/release var-require-all-GITHUB_TOKEN | |||
| @bin/release hashrelease build | |||
| @bin/release hashrelease publish | |||
| @bin/release hashrelease build ${BUILD_ARGS} | |||
There was a problem hiding this comment.
Can we just decouple the hashrelease.yml Semaphore file from the Makefile? I don't think there's any reason it needs to do this complicated passthrough of CLI flags via env vars.
It can just call bin/release directly, passing the arguments that it wants.
There was a problem hiding this comment.
Alternatively, we can add ENV equivalents for all of the CLI args like we have done for some, and then we can completely configure the CLI arguments via env vars when running in CI.
There was a problem hiding this comment.
will switch to ENV.
I am using the make target make hashrelease so that the variables from metdata.mk file gets used
release/build/main.go
Outdated
| @@ -160,7 +162,7 @@ func hashreleaseSubCommands(cfg *config.Config) []*cli.Command { | |||
| &cli.BoolFlag{Name: skipValidationFlag, Usage: "Skip all pre-build validation", Value: false}, | |||
| &cli.BoolFlag{Name: skipBranchCheckFlag, Usage: "Skip check that this is a valid release branch.", Value: false}, | |||
| &cli.BoolFlag{Name: buildImagesFlag, Usage: "Build images from local codebase. If false, will use images from CI instead.", Value: false}, | |||
| &cli.StringFlag{Name: imageRegistryFlag, Usage: "Specify image registry to use", Value: ""}, | |||
| &cli.StringFlag{Name: imageRegistryFlag, Usage: "Specify image registry to use", EnvVars: []string{"REGISTRIES", "DEV_REGISTRIES"}, Value: ""}, | |||
There was a problem hiding this comment.
Do we need DEV_REGISTRIES? I think if nobody is using it we can just have a single env var for this.
There was a problem hiding this comment.
will switch to just REGISTRIES
| return fmt.Sprintf("https://%s.%s", h.Name, BaseDomain) | ||
| } | ||
|
|
||
| func (h *Hashrelease) AsLatest() *Hashrelease { |
There was a problem hiding this comment.
This functions seems unnecessary to me - we already have a public Latest field on the struct, so the calling code can just do hr.Latest = true which is much easier to read.
| @@ -63,11 +66,41 @@ func WithOutputDir(outputDir string) Option { | |||
| } | |||
| } | |||
|
|
|||
| func WithPublishOptions(images, tag, github bool) Option { | |||
| func WithPublishOptions(opt ...PublishOptions) Option { | |||
There was a problem hiding this comment.
I don't think I like this pattern of nested options. We should just define Option options for all of the things that we want to make configurable. We don't need an option to specify multiple other options. Or do we? Is there something I'm missing?
There was a problem hiding this comment.
I think this makes the configuration a bit more versatile across hashrelease and release. As well as it works better for close source.
There was a problem hiding this comment.
As well as it works better for close source.
Could you explain this a bit more? It's not obvious to me how this is better for closed source code.
| } | ||
| } | ||
|
|
||
| func PublishImages() PublishOptions { |
There was a problem hiding this comment.
| func PublishImages() PublishOptions { | |
| func PublishImages() Option { |
| if err != nil { | ||
| return fmt.Errorf("failed to get tag for checked-out commit, is there one? %s", err) | ||
| } | ||
| func (r *CalicoManager) publishToHashreleaseServer() error { |
There was a problem hiding this comment.
I don't think it makes sense to do this degree of wrapping - either we make the CalicoManager truly responsible for publishing hashreleases and get rid of hashreleaseserver.PublishHashrelease, or we leave them separate.
I think it's confusing if we're passing configuration to the CalicoManager, just for it to turn around and call the task-based functions in the hashreleaseserver package.
| @@ -48,10 +49,6 @@ func NewManager(opts ...Option) *CalicoManager { | |||
| b := &CalicoManager{ | |||
| runner: &command.RealCommandRunner{}, | |||
| validate: true, | |||
| buildImages: true, | |||
There was a problem hiding this comment.
I think these should stay the default for a real release. Hashrelease can override them, no?
There was a problem hiding this comment.
the options only allow setting true.
With release we already pass the value as true
| @@ -477,6 +468,9 @@ func (r *CalicoManager) releasePrereqs() error { | |||
|
|
|||
| // Prerequisites specific to publishing a release. | |||
| func (r *CalicoManager) publishPrereqs() error { | |||
| if r.isHashRelease { | |||
| return nil | |||
There was a problem hiding this comment.
Do we not have any other hashrelease prereqs? e.g., if it already exists on the server?
I think we're checking that elsewhere, but why not here? Seems better to consolidate into one location. As it stands, it's not obvious when something belongs in the CalicoManager vs. when it just goes into main.go.
There was a problem hiding this comment.
I can add the check here. However it is in main so that operator images do not get pushed if the hashrelease already exist
| @@ -708,7 +708,8 @@ func (r *CalicoManager) buildContainerImages(ver string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (r *CalicoManager) publishGithubRelease(ver string) error { | |||
| func (r *CalicoManager) publishGithubRelease() error { | |||
| ver := r.calicoVersion | |||
There was a problem hiding this comment.
We should just swap ver for r.calicoVersion instead of using a local variable.
- key-cert-provisioner: add release-build and release-publish targets - add ability to skip release notes
- allow changing registry in pinnedversion - allowing skipping validation for publish - move hashrelease validation to calico manager - update .gitignore to ignore publishing identifier files - fix key-cert-provisioner publishing
Description
This allows building and publishing images in hashreleases.
Semaphore allows the ability to distinguish how a workflow is started using
SEMAPHORE_WORKFLOW_TRIGGERED_BY_SCHEDULEenvironment variable.This allow cleans up some of the hashrelease tasks and move into calico manager or build/main.go
Related issues/PRs
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.