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

Rebuild containernetworking binaries, so we don't need to create rele… #8909

Merged
merged 7 commits into from
Jul 4, 2024

Conversation

rene-dekker
Copy link
Contributor

@rene-dekker rene-dekker commented Jun 13, 2024

We never moved https://github.com/projectcalico/containernetworking-plugins to the monorepo, because the long-term vision is to use upstream. Because of this, we need to make a new release to this project every time a new golang patch comes out.

I added some automation to download the code of the project and rebuild it with go-build from the monorepo, thereby removing this tedious step of creating new releases and updating the makefile with the new version.

@rene-dekker rene-dekker requested a review from a team as a code owner June 13, 2024 19:49
@marvin-tigera marvin-tigera added this to the Calico v3.29.0 milestone Jun 13, 2024
@marvin-tigera marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Jun 13, 2024
Copy link
Member

@fasaxc fasaxc left a comment

Choose a reason for hiding this comment

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

I think you need to make sure that updates to the CNI_VERSION trigger a rebuild.

cni-plugin/Makefile Show resolved Hide resolved
git clone --single-branch --branch $(CNI_VERSION) https://github.com/projectcalico/containernetworking-plugins.git
touch $@

$(CONTAINERNETWORKING_PLUGINS_CREATED): $(CONTAINERNETWORKING_PLUGINS_CLONED)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure it's any cleaner(!) but there's a way to tell make that this target makes several binaries on a single invocation:

.../bin/foo .../bin/bar .../bin/baz &: $(TOUCHFILE)
    ...

The &: means "one call to this creates all these files"

I used that in the BPF builds to avoid issues when calling through to the BPF makefile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took over your suggestion. I think it does look slightly cleaner now. Thanks for teaching me this feature.

@rene-dekker
Copy link
Contributor Author

I think you need to make sure that updates to the CNI_VERSION trigger a rebuild.

I'll make the changes, but don't think the necessity is that high. The binaries are built on every merge job and so the (hash) releases will always have the latest go. Other than golang updates, this repo has been dead for years. For the same reason do I think that we don't need to ever update the CNI_VERSION any longer.

cni-plugin/Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@coutinhop coutinhop left a comment

Choose a reason for hiding this comment

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

LGTM

@fasaxc
Copy link
Member

fasaxc commented Jun 24, 2024

I'll make the changes, but don't think the necessity is that high. The binaries are built on every merge job and so the (hash) releases will always have the latest go.

Thanks, you're right that release builds should always be clean, but developers use makefiles too, and it's not sustainable for us to maintain an ever growing mental list of "things the makefile can't handle". I really don't want to have to run make clean every time I switch branches.

@coutinhop coutinhop merged commit c86e1b7 into projectcalico:master Jul 4, 2024
2 checks passed
@rene-dekker rene-dekker deleted the ev-4897 branch July 4, 2024 18:12
matthewdupre added a commit that referenced this pull request Jul 4, 2024
…909-origin-release-v3.28

Automated cherry pick of #8909: Rebuild containernetworking binaries, so we don't need to
coutinhop added a commit to coutinhop/calico that referenced this pull request Jul 8, 2024
Do the same for flannel-cni-plugin as projectcalico#8909
did for containernetworking-plugins, i.e. clone the repo and build using
the go-build image from the calico monorepo on-demand.

This is done to remove the tedious step of creating new releases and updating
the Makefile with the new version on the flannel-cni-plugin repo.
coutinhop added a commit to coutinhop/calico that referenced this pull request Jul 8, 2024
Do the same for flannel-cni-plugin as projectcalico#8909
did for containernetworking-plugins, i.e. clone the repo and build using
the go-build image from the calico monorepo on-demand.

This is done to remove the tedious step of creating new releases and updating
the Makefile with the new version on the flannel-cni-plugin repo.
coutinhop added a commit that referenced this pull request Jul 9, 2024
…909-origin-release-v3.27

Automated cherry pick of #8909: Rebuild containernetworking binaries, so we don't need to
coutinhop added a commit to coutinhop/calico that referenced this pull request Jul 9, 2024
Do the same for flannel-cni-plugin as projectcalico#8909
did for containernetworking-plugins, i.e. clone the repo and build using
the go-build image from the calico monorepo on-demand.

This is done to remove the tedious step of creating new releases and updating
the Makefile with the new version on the flannel-cni-plugin repo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-pr-required Change is not yet documented release-note-required Change has user-facing impact (no matter how small)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants