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

Added an option to disable namespace-metadata #11782

Merged
merged 10 commits into from
Jan 22, 2024
Merged

Conversation

shinigami-777
Copy link
Contributor

@shinigami-777 shinigami-777 commented Dec 18, 2023

Fixes #11585
Added option in values.yaml in extensions charts to disable the namspace-metadata jobs for helm based installations.
The createNamespaceMetadataJob flag should be set to false from cli to disable it. Disable if lack of privileges require doing it manually.

Signed-off-by: shinigami-777 <chattopadhyaytamaghna@gmail.com>
Signed-off-by: shinigami-777 <chattopadhyaytamaghna@gmail.com>
@shinigami-777 shinigami-777 requested a review from a team as a code owner December 18, 2023 15:04
@shinigami-777 shinigami-777 changed the title Added an option to disable namespace-metadata #11585 Added an option to disable namespace-metadata Dec 18, 2023
@shinigami-777
Copy link
Contributor Author

Providing namespace-metadata disable option #11585

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Hi @shinigami-777 , thanks for the contribution. Please add a comment to this PR describing your change, and a reference to the issue it fixes. Also make sure you test your change before submitting, to validate it addresses the issue.

shinigami-777 and others added 2 commits December 19, 2023 15:56
Signed-off-by: shinigami-777 <chattopadhyaytamaghna@gmail.com>
shinigami-777 and others added 2 commits December 22, 2023 16:28
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

I've added a suggestion for a different naming of this new setting. After applying that, please run ./bin/helm-docs to regenerate the helm values documentation in the charts README.md files.

Comment on lines 35 to 36
# -- adding an option to disble namespace-metadata jobs
skipNamespaceMetadataJobs: false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# -- adding an option to disble namespace-metadata jobs
skipNamespaceMetadataJobs: false
# -- Creates a Job that adds necessary metadata to the extension's namespace
# during install; disable if lack of privileges require doing this manually
createNamespaceMetadataJobs: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion @alpeb .I have made the suggested changes and regenerated the helm values documentation in the charts README.md files.

…ntation in charts README.md files

Signed-off-by: shinigami-777 <chattopadhyaytamaghna@gmail.com>
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Thanks @shinigami-777 , this looks good to me. I'd like however to amend my previous comment on naming: can you rename the setting createNamespaceMetadataJob, given that it's just one job (per extension)? Sorry for the nit picking!

@@ -76,3 +77,4 @@ subjects:
- kind: ServiceAccount
name: namespace-metadata
namespace: {{.Release.Namespace}}
{{-end}}
Copy link
Member

Choose a reason for hiding this comment

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

You require spaces here or else the template breaks, as you can see in the integration tests results.

Suggested change
{{-end}}
{{- end }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alpeb . Added spaces between those end statements.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, can you address my comment about naming above? After that, this PR looks fine from my perspective.

Signed-off-by: shinigami-777 <chattopadhyaytamaghna@gmail.com>
Signed-off-by: shinigami-777 <chattopadhyaytamaghna@gmail.com>
@shinigami-777
Copy link
Contributor Author

Thanks @shinigami-777 , this looks good to me. I'd like however to amend my previous comment on naming: can you rename the setting createNamespaceMetadataJob, given that it's just one job (per extension)? Sorry for the nit picking!

Yes I have renamed the setting createNamespaceMetadataJob from skipNamespaceMetadataJobs in the extensions files.

@alpeb
Copy link
Member

alpeb commented Jan 3, 2024

Yes I have renamed the setting createNamespaceMetadataJob from skipNamespaceMetadataJobs in the extensions files.

I still see skipNamespaceMetadataJobs in the diff...

@shinigami-777
Copy link
Contributor Author

shinigami-777 commented Jan 4, 2024

I still see skipNamespaceMetadataJobs in the diff...

I can't see the skipNamespaceMetadataJobs in the changed files. Can you please point me to where it is.

@alpeb
Copy link
Member

alpeb commented Jan 4, 2024

I meant to say the setting should be createNamespaceMetadataJob instead of createNamespaceMetadataJobs.

Signed-off-by: shinigami-777 <chattopadhyaytamaghna@gmail.com>
@shinigami-777
Copy link
Contributor Author

I meant to say the setting should be createNamespaceMetadataJob instead of createNamespaceMetadataJobs.

Yes I have updated it to createNamespaceMetadataJob from createNamespaceMetadataJobs.

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Thanks @shinigami-777 , this looks good to me!

@shinigami-777
Copy link
Contributor Author

Thanks @shinigami-777 , this looks good to me!

Thanks @alpeb for the guidance.

@adleong adleong merged commit 9818cb2 into linkerd:main Jan 22, 2024
33 checks passed
@olix0r olix0r mentioned this pull request Jan 26, 2024
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.

Provide option to disable namespace-metadata Job
3 participants