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

Adding AdditionalLabels #236

Merged
merged 13 commits into from
Dec 3, 2021
Merged

Adding AdditionalLabels #236

merged 13 commits into from
Dec 3, 2021

Conversation

piclemx
Copy link
Contributor

@piclemx piclemx commented Nov 17, 2021

What this PR does:
Adding the possibility to people to add Labels and the all objects that will be created by the operator.

Which issue(s) this PR fixes:
Fixes #235

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CHANGELOG.md updated (not required for documentation PRs)
  • CLA Signed: DataStax CLA

@piclemx piclemx requested a review from a team as a code owner November 17, 2021 16:00
@jsanda jsanda requested review from jsanda and removed request for a team November 17, 2021 18:21
@piclemx piclemx changed the title [WIP] Adding AdditionalLabels and AdditionalAnnotations Adding AdditionalLabels and AdditionalAnnotations Nov 17, 2021
Copy link
Contributor

@jsanda jsanda left a comment

Choose a reason for hiding this comment

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

You're off to a good start. There are some other objects that the operator like the superuser secret and endpoints for the additional seeds service. There might be more, but those are the ones I know of immediately.

Are you going to add corresponding functionality for annotations?

apis/cassandra/v1beta1/cassandradatacenter_types.go Outdated Show resolved Hide resolved
pkg/oplabels/labels.go Show resolved Hide resolved
pkg/reconciliation/construct_service_test.go Outdated Show resolved Hide resolved
@piclemx
Copy link
Contributor Author

piclemx commented Nov 22, 2021

You're off to a good start. There are some other objects that the operator like the superuser secret and endpoints for the additional seeds service. There might be more, but those are the ones I know of immediately.

Are you going to add corresponding functionality for annotations?

I think for now I will only add the labels for the first step. I can do the annotations later if you want? If you want both of them now I can also work on getting both.

@piclemx
Copy link
Contributor Author

piclemx commented Nov 22, 2021

I will be adding the missing test tomorrow.

@piclemx
Copy link
Contributor Author

piclemx commented Nov 24, 2021

@jsanda @burmanm could you do another round of review?

For the annotation, do you mind I do that in another merge request?

@piclemx piclemx changed the title Adding AdditionalLabels and AdditionalAnnotations Adding AdditionalLabels Nov 24, 2021
@piclemx piclemx requested a review from jsanda November 24, 2021 16:44
@piclemx piclemx requested a review from burmanm November 24, 2021 21:05
@piclemx
Copy link
Contributor Author

piclemx commented Nov 30, 2021

@jsanda @burmanm do you still have sometime to review this one 🥺

@jsanda
Copy link
Contributor

jsanda commented Nov 30, 2021

Last week was a really short week for me due to the Thanksgiving holiday, and I just didn't have time to get back to it. I will do another review today or tomorrow.

Copy link
Contributor

@jsanda jsanda left a comment

Choose a reason for hiding this comment

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

My apologies for the delayed review. The changes look good. There is another place you need to update for the default super user secret. See here.

@piclemx
Copy link
Contributor Author

piclemx commented Dec 2, 2021

My apologies for the delayed review. The changes look good. There is another place you need to update for the default super user secret. See here.

Does it make sense to use the AddOperatorLabels there also?

You don't need to apologies @jsanda ;)

@jsanda
Copy link
Contributor

jsanda commented Dec 2, 2021

Does it make sense to use the AddOperatorLabels there also?

yeah, I think so

@piclemx
Copy link
Contributor Author

piclemx commented Dec 2, 2021

Does it make sense to use the AddOperatorLabels there also?

yeah, I think so

Done

@piclemx piclemx requested a review from jsanda December 2, 2021 16:12
Copy link
Contributor

@jsanda jsanda 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 adding the changes for the superuser secret. Pending the test run, I think it is good to go :)

@piclemx
Copy link
Contributor Author

piclemx commented Dec 3, 2021

Nice thank you ;)

Copy link
Contributor

@jsanda jsanda left a comment

Choose a reason for hiding this comment

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

Thanks!

@jsanda jsanda merged commit 0a85469 into k8ssandra:master Dec 3, 2021
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.

K8SSAND-1052 ⁃ No options to add labels on the statefulset object
3 participants