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 Affinity for Cruise Control #962

Merged
merged 3 commits into from Apr 24, 2023
Merged

Conversation

shubhamcoc
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
API breaks? no
Deprecations? no
Related tickets #844
License Apache 2.0

Description

This PR is meant to add the option of specifying an Affinity configuration for Cruise Control pod.

Type of Change

  • New Feature

Checklist

  • I have read the contributing guidelines
  • Existing issues have been referenced (where applicable)
  • I have verified this change is not present in other open pull requests
  • All new and existing tests pass

@shubhamcoc shubhamcoc requested a review from a team as a code owner April 20, 2023 15:29
@CLAassistant
Copy link

CLAassistant commented Apr 20, 2023

CLA assistant check
All committers have signed the CLA.

@panyuenlau
Copy link
Member

Hey @shubhamcoc, thank you for your contribution!

Quick comment - looks like you copied what we have under the api/ directory to a new directory pkg/sdk/ and it introduces a lot of unnecessary changes in the PR. Is this intended?

@shubhamcoc
Copy link
Contributor Author

Hi, Yes I have done it because locally the test was failing, so I have to replace api package with ./pkg/sdk/ in go.mod. I can remove it, but not sure if tests will pass here, so wanted to ask how should we test it?

@panyuenlau
Copy link
Member

Hi, Yes I have done it because locally the test was failing, so I have to replace api package with ./pkg/sdk/ in go.mod. I can remove it, but not sure if tests will pass here, so wanted to ask how should we test it?

Got you. What I do whenever I need to make changes to the api directory for development is to add this line to the root go.mod:

github.com/banzaicloud/koperator => ./api

This tells Go to use the ./api from your local machine instead of the tagged version defined here

Note: remember to remove this line when you submit PRs


When you submit PR, you need to do the following steps:

  1. Submit one PR for the changes within the api directory. We will create a tag once the PR is merged so the newly introduced changes can be used in the next step
  2. Then submit another PR for the rest of the changes, and this PR would upgrade to the new API version tag that we released in step 1 (e.g. upgrading here to the new tag version)

We are aware that the current development process is quite cumbersome and we are planning to improve it.

Please feel free to join our Slack space where we can provide help in a more timely manner

@shubhamcoc
Copy link
Contributor Author

Hi @panyuenlau, I have kept the changes related to api packages in the current patch. Kindly review it.

Copy link
Member

@panyuenlau panyuenlau left a comment

Choose a reason for hiding this comment

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

looks good, thanks

@shubhamcoc
Copy link
Contributor Author

Hi @panyuenlau, Looks like I will need an invitation to join your slack channel. Can you provide an invitation?

@panyuenlau
Copy link
Member

Hi @panyuenlau, Looks like I will need an invitation to join your slack channel. Can you provide an invitation?

@shubhamcoc Thanks for reporting the issue to us, we weren't aware that now it would take an invitation to join our Slack.

I've just sent an invitation to shubhamkar77@gmail.com, let me know if I should send to a different address

@panyuenlau panyuenlau merged commit c94e166 into banzaicloud:master Apr 24, 2023
4 checks passed
@panyuenlau
Copy link
Member

Hey @shubhamcoc, the new API tag https://github.com/banzaicloud/koperator/releases/tag/api%2Fv0.26.0 has been released, looking forward to your PR about the implementation!

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.

None yet

5 participants