Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Move seed pod labelling to a separate controller #270

Merged
merged 1 commit into from
Mar 1, 2018
Merged

Move seed pod labelling to a separate controller #270

merged 1 commit into from
Mar 1, 2018

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Feb 28, 2018

Release note:

NONE

kragniz
kragniz approved these changes Feb 28, 2018
View reviewed changes
Copy link
Contributor

@kragniz kragniz left a comment

Choose a reason for hiding this comment

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

Looks great, I'll merge once e2e tests are passing

pkg/controllers/cassandra/service/seedprovider/resource.go
@@ -10,6 +10,8 @@ import (

const (
TolerateUnreadyEndpointsAnnotationKey = "service.alpha.kubernetes.io/tolerate-unready-endpoints"
SeedLabelKey = "navigator.jetstack.io/cassandra-seed"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 this name is more explicit

@wallrj
Copy link
Member Author

wallrj commented Feb 28, 2018

Test failures were caused by single double quoted map keys in the jsonpath expression.
I've made them single quoted now.
With an idea for a future improvement to the E2E tests for seed node service.

@wallrj
Copy link
Member Author

wallrj commented Feb 28, 2018

Try again, without the funky shell quoting.

@wallrj
Move seed pod labelling to a separate controller
e23ce4e 
* In jetstack#256 I want to refactor nodepool.Sync so that its only responsibility is to update the NodePool status.
* And the seed labelling seems like it is a separate concern that can live in its own module.
* With its own unit tests.
@wallrj
Copy link
Member Author

wallrj commented Feb 28, 2018

Use a label selector instead.
And do the test after scaling out the cluster.

@kragniz
Copy link
Contributor

kragniz commented Feb 28, 2018

/retest

1 similar comment
@kragniz
Copy link
Contributor

kragniz commented Feb 28, 2018

/retest

kragniz
kragniz approved these changes Mar 1, 2018
View reviewed changes
Copy link
Contributor

@kragniz kragniz left a comment

Choose a reason for hiding this comment

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

/lgtm

@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kragniz

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@jetstack-ci-bot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@kragniz kragniz mentioned this pull request Mar 1, 2018
Open
@jetstack-ci-bot
Copy link
Contributor

Automatic merge from submit-queue.

@jetstack-ci-bot jetstack-ci-bot merged commit 2ad596b into jetstack:master Mar 1, 2018
@wallrj wallrj deleted the seed-pod-label-controller branch March 6, 2018 09:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kragniz kragniz kragniz approved these changes

@munnerz munnerz Awaiting requested review from munnerz

Assignees

@kragniz kragniz

Labels
approved lgtm release-note-none size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wallrj @kragniz @jetstack-bot @jetstack-ci-bot