Skip to content

Remove use of intermediate build image #8230

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Remove use of intermediate build image #8230

wants to merge 1 commit into from

Conversation

bryantbiggs
Copy link
Contributor

@bryantbiggs bryantbiggs commented Feb 20, 2025 •
edited
Loading

Description

  • Round two of Upgrade to go 1.22 #8125; removing the intermediate build image
  • The .requirements file has been removed; binaries/libs required for building/testing have been placed under the install-tools make command. Future: investigate using go tools ... which was added in 1.24
  • The build/docker/* directory and associated files are removed, removing the intermediate build image artifacts
    • This removes the need for the .github/workflows/ecr-publish-build.yaml workflow
    • This removes the need for Makefile.docker
  • .github/workflows/build-all-distros-nightly.yaml workflow is removed since its redundant of the CI checks, and has been failing for some time

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@bryantbiggs bryantbiggs added skip-release-notes Causes PR not to show in release notes area/tech-debt Leftover improvements in code, testing and building labels Feb 20, 2025
@bryantbiggs bryantbiggs enabled auto-merge (squash) February 20, 2025 15:08
@bryantbiggs bryantbiggs mentioned this pull request Feb 20, 2025
Merged
7 tasks
@bryantbiggs bryantbiggs force-pushed the chore/remove-build-image branch from 10a2a64 to fc8961b Compare February 20, 2025 17:46
bryantbiggs
bryantbiggs commented Feb 20, 2025
View reviewed changes
.github/workflows/release-candidate.yaml Outdated
@@ -7,22 +7,14 @@ jobs:
rc:
name: Push release candidate tag
runs-on: ubuntu-latest
container: public.ecr.aws/eksctl/eksctl-build:26e3c36557da8ae2db5d995cea673e05cc60cfce
Copy link
Contributor Author

@bryantbiggs bryantbiggs Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

removing the build image was previously reverted because these container references were not cleaned up from the CI scripts, which caused the subsequent release to fail due to image not found

cheeseandcereal
cheeseandcereal requested changes Feb 21, 2025
View reviewed changes
Copy link
Member

@cheeseandcereal cheeseandcereal left a comment

Choose a reason for hiding this comment

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

Changes also need to be made in the other repos in this org to support these changes

Makefile Outdated
@@ -33,11 +32,15 @@ endif

##@ Dependencies
.PHONY: install-all-deps
install-all-deps: install-build-deps install-site-deps ## Install all dependencies for building both binary and user docs)
install-all-deps: install-site-deps ## Install all dependencies for building both binary and user docs)
Copy link
Member

Choose a reason for hiding this comment

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

why is install-all-deps no longer installing the other dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this looks like a mistake - will re-add

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

Makefile Outdated
Comment on lines 37 to 38
.PHONY: install-tools
install-tools: ## Install dependencies for code generation and test execution
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for renaming this target? There are other references to the old target name install-build-deps in various other places that also need to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its a bit mis-leading, the aren't just build deps but also deps required for testing (cert generation), and this matched the tools.go a bit better. I can go either way though - let me know which way we want go and I'll make sure any references are updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted

@bryantbiggs bryantbiggs marked this pull request as draft February 22, 2025 00:21
auto-merge was automatically disabled February 22, 2025 00:21

Pull request was converted to draft

This was referenced Feb 22, 2025
Merged
Merged
@bryantbiggs bryantbiggs force-pushed the chore/remove-build-image branch from e1dac30 to 959d51d Compare February 22, 2025 16:40
@bryantbiggs bryantbiggs deleted the chore/remove-build-image branch February 28, 2025 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheeseandcereal cheeseandcereal cheeseandcereal requested changes

Assignees
No one assigned
Labels
area/tech-debt Leftover improvements in code, testing and building skip-release-notes Causes PR not to show in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bryantbiggs @cheeseandcereal