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

Parametrize job cluster role and image #214

Merged
merged 6 commits into from
Dec 12, 2023

Conversation

piotrminkina
Copy link
Contributor

Mainly implements #213 and additionally allows also specyfiying Cluster Role to be used with job images. These are changes that are needed in a hermetic, corporate environment with no direct access to the Internet and a zero-trust policy. I also made other improvements that I felt were appropriate, as a fresh person joining the project.

Constructive comments welcome — I'm a Golang newbie, albeit with a non-short programming experience.

@piotrminkina piotrminkina force-pushed the feature/parametrize-controller branch 2 times, most recently from 82435aa to fb310df Compare November 29, 2023 19:31
@piotrminkina
Copy link
Contributor Author

How to solve the reported problems that are not the result of my changes?

test/suite/zz_suite_test.go:6:2: dot-imports: should not use dot imports (revive)
	. "github.com/onsi/ginkgo/v2"
	^
test/suite/zz_suite_test.go:7:2: dot-imports: should not use dot imports (revive)
	. "github.com/onsi/gomega"
	^
test/suite/helm_test.go:7:2: dot-imports: should not use dot imports (revive)
	. "github.com/onsi/ginkgo/v2"
	^

Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! A couple nits but overall looks good.

main.go Outdated Show resolved Hide resolved
pkg/controllers/chart/chart.go Outdated Show resolved Hide resolved
@piotrminkina piotrminkina force-pushed the feature/parametrize-controller branch from ab2c47b to e9b04c8 Compare November 30, 2023 10:28
@piotrminkina piotrminkina force-pushed the feature/parametrize-controller branch from e9b04c8 to 1fea2d6 Compare November 30, 2023 12:07
@piotrminkina piotrminkina force-pushed the feature/parametrize-controller branch from 1fea2d6 to e1c93f5 Compare November 30, 2023 12:13
@brandond
Copy link
Member

LGTM, but tests appear to be failing due to the linter complaining about dot imports of the test DSL... if you want to take a shot at fixing that go for it, otherwise I'll fix it in another PR and you can rebase.

@piotrminkina piotrminkina force-pushed the feature/parametrize-controller branch from 80068ea to 5c32c60 Compare December 1, 2023 16:32
@piotrminkina
Copy link
Contributor Author

@brandond I added an exclusion rule, exactly as they did in Istio: https://github.com/istio/client-go/blob/46ba363b2dc16d8cd575f17de240ca840db2f7cf/common/config/.golangci.yml#L259

@piotrminkina
Copy link
Contributor Author

@brandond I can't merge. Will You plan?

@brandond brandond merged commit cf4b87d into k3s-io:master Dec 12, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants