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

[v2] Consolidate go.mod #686

Merged
merged 8 commits into from
Sep 26, 2024
Merged

[v2] Consolidate go.mod #686

merged 8 commits into from
Sep 26, 2024

Conversation

blampe
Copy link
Contributor

@blampe blampe commented Sep 25, 2024

  • Consolidate {operator,agent}/go.mod under a root go.mod.
  • Rewrite imports to use v2 path.
  • Remove /test -- these were still referring to v1 code and can be revived in a followup if we want to keep any of them.
  • Move Dockerfile to repo root and fix image build.

Makefile targets are unchanged except for a small rename docker-rootless → docker-nonroot.

Fixes #687.

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.11%. Comparing base (fcd9589) to head (59a35e6).
Report is 1 commits behind head on v2.

Additional details and impacted files
@@           Coverage Diff           @@
##               v2     #686   +/-   ##
=======================================
  Coverage   49.11%   49.11%           
=======================================
  Files          26       26           
  Lines        2877     2877           
=======================================
  Hits         1413     1413           
  Misses       1287     1287           
  Partials      177      177           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blampe blampe added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Sep 25, 2024
Copy link
Contributor

@EronWright EronWright left a comment

Choose a reason for hiding this comment

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

I agree with removing the tests/ folder because it seems obsolete.

Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@EronWright EronWright left a comment

Choose a reason for hiding this comment

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

I just saw that the Dockerfile was moved, which is fine but please update the Makefile and goreleaser file accordingly.

There's targets docker-build docker-buildx and bundle to consider. I suppose you could move some stuff down to the root makefile. The 'bundle' images are, I suppose, most obviously tied to the operator and bundle target seems to belong in operator makefile.

@blampe
Copy link
Contributor Author

blampe commented Sep 26, 2024

I just saw that the Dockerfile was moved, which is fine but please update the Makefile and goreleaser file accordingly.

There's targets docker-build docker-buildx and bundle to consider. I suppose you could move some stuff down to the root makefile. The 'bundle' images are, I suppose, most obviously tied to the operator and bundle target seems to belong in operator makefile.

Good call @EronWright I fixed pointed goreleader and docker-buildx at the new location. I think it would make sense to eventually move most (all?) of the operator's Makefile to the root but it's not urgent.

@blampe blampe requested a review from EronWright September 26, 2024 17:37
operator/Makefile Outdated Show resolved Hide resolved
@@ -74,7 +74,7 @@ dockers:
goarch: amd64

# Path to the Dockerfile (from the project root).
dockerfile: operator/Dockerfile
dockerfile: Dockerfile
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that goreleaser doesn't provide the necessary context or VERSION parameter for our Dockerfile, so this is probably not working.

Copy link
Contributor Author

@blampe blampe Sep 26, 2024

Choose a reason for hiding this comment

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

Yeah I also realized we're skipping the docker step during the dry run? Oh well not a big deal since we'll be getting rid of this anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should remove the dockers section of the goreleaser if it isn't expected to work.

Co-authored-by: Eron Wright <[email protected]>
@blampe blampe merged commit 7ce746c into v2 Sep 26, 2024
7 checks passed
@blampe blampe deleted the blampe/v2-go.mod branch September 26, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants