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.

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

Allow overriding konnectivity parameters #411

Merged
merged 2 commits into from
Mar 4, 2024
Merged

Allow overriding konnectivity parameters #411

merged 2 commits into from
Mar 4, 2024

Conversation

jwitko
Copy link
Contributor

@jwitko jwitko commented Feb 8, 2024

Fixes #410

This will allow us to override konnectivity hard-coded defaults using the existing extraArgs parameter.

@netlify Netlify
Copy link

netlify bot commented Feb 8, 2024 •
edited
Loading

✅ Deploy Preview for kamaji-documentation ready!

Name Link
🔨 Latest commit 9ab29fc
🔍 Latest deploy log https://app.netlify.com/sites/kamaji-documentation/deploys/65de150191ea390008c0074f
😎 Deploy Preview https://deploy-preview-411--kamaji-documentation.netlify.app/reference/api
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

prometherion
prometherion requested changes Feb 8, 2024
View reviewed changes
Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

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

May I ask you to follow the semantic commit message convention, please?

@jwitko jwitko force-pushed the 410-Allow-konnectivity-parameter-override branch from d5bd6ec to a949dc2 Compare February 8, 2024 21:24
@jwitko
Copy link
Contributor Author

jwitko commented Feb 8, 2024

May I ask you to follow the semantic commit message convention, please?

will do!

@jwitko jwitko requested a review from prometherion February 8, 2024 21:35
prometherion
prometherion requested changes Feb 8, 2024
View reviewed changes
Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

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

I would mark this commit as a fix:
fix: konnectivity extra args override

@prometherion
Copy link
Member

If the suite is green I can reword the commit, no worries.

@jwitko
Copy link
Contributor Author

jwitko commented Feb 8, 2024

If the suite is green I can reword the commit, no worries.

Found some issues locally. Will come back with reworded commit and working tests.

@jwitko jwitko force-pushed the 410-Allow-konnectivity-parameter-override branch from a949dc2 to 8494598 Compare February 8, 2024 22:48
@jwitko
Copy link
Contributor Author

jwitko commented Feb 8, 2024

@prometherion OK did a bit of reworking. Now allowing the address to be set via TenantControlPlane.spec.addons.konnectivity.server.address but also still overriding agent arguments via extraArgs.

Open to any comments/concerns/questions.

@jwitko jwitko requested a review from prometherion February 8, 2024 22:50
prometherion
prometherion requested changes Feb 13, 2024
View reviewed changes
Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

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

Minor changes and we can get this merged! 🚀

api/v1alpha1/tenantcontrolplane_types.go Outdated Show resolved Hide resolved
config/crd/bases/kamaji.clastix.io_tenantcontrolplanes.yaml Outdated Show resolved Hide resolved
internal/resources/konnectivity/agent.go Outdated Show resolved Hide resolved
internal/resources/konnectivity/agent.go Show resolved Hide resolved
@jwitko jwitko force-pushed the 410-Allow-konnectivity-parameter-override branch from 8494598 to 6e7773c Compare February 13, 2024 19:22
@jwitko jwitko requested a review from prometherion February 13, 2024 19:25
@jwitko jwitko force-pushed the 410-Allow-konnectivity-parameter-override branch from 6e7773c to 6c69f37 Compare February 14, 2024 16:09
@jwitko jwitko requested a review from prometherion February 14, 2024 16:10
@jwitko jwitko force-pushed the 410-Allow-konnectivity-parameter-override branch from 6c69f37 to 1310aca Compare February 19, 2024 15:48
@jwitko
Copy link
Contributor Author

jwitko commented Feb 19, 2024

After realizing the CRDs are generated from #414 I modified this PR to also generate the CRD changes from the code comments.

prometherion
prometherion requested changes Feb 26, 2024
View reviewed changes
Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

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

Please, may I ask you to commit the changes generated by make yaml-installation-file?

@jwitko jwitko force-pushed the 410-Allow-konnectivity-parameter-override branch from 1310aca to f62ce15 Compare February 26, 2024 22:23
@jwitko
Copy link
Contributor Author

jwitko commented Feb 26, 2024

Please, may I ask you to commit the changes generated by make yaml-installation-file?

Yes of course, it's done now.

@jwitko jwitko requested a review from prometherion February 26, 2024 22:24
@prometherion prometherion added this to the v0.4.2 milestone Feb 27, 2024
@prometherion prometherion merged commit cec4f91 into clastix:master Mar 4, 2024
10 checks passed
@jwitko jwitko deleted the 410-Allow-konnectivity-parameter-override branch March 4, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prometherion prometherion prometherion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.4.2
Development

Successfully merging this pull request may close these issues.

Allow for overriding konnectivity parameters
2 participants
@jwitko @prometherion