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

Create a dedicated module for api #108

Closed
wants to merge 3 commits into from
Closed

Create a dedicated module for api #108

wants to merge 3 commits into from

Conversation

mvladev
Copy link

@mvladev mvladev commented Oct 26, 2020 •
edited
Loading

What this PR does / why we need it:

Reduces dependency hell

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

I've split the PR into 3 separate commits to make the review easier.

Release note:

A new module `github.com/gardener/etcd-druid/api` can be used to get the API definitions.

Switch to `github.com/gardener/etcd-druid-api` if you vendor only the API of etcd-druid.

@gardener-robot gardener-robot added the needs/review Needs review label Oct 26, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 26, 2020
@gardener-robot gardener-robot added size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Oct 26, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 26, 2020
@amshuman-kr
Copy link
Collaborator

@mvladev I like it. It makes sense. This should ideally be the general pattern for most controllers that have resource definitions, right?

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 2, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 2, 2020
mvladev
mvladev commented Nov 2, 2020
View reviewed changes
.ci/pipeline_definitions Outdated Show resolved Hide resolved
@gardener-robot gardener-robot removed the needs/review Needs review label Nov 2, 2020
@mvladev mvladev marked this pull request as ready for review November 2, 2020 23:06
@mvladev mvladev requested a review from a team as a code owner November 2, 2020 23:06
@mvladev mvladev changed the title [DO NOT MERGE] Create a dedicated module for api Create a dedicated module for api Nov 2, 2020
Martin Vladev added 2 commits November 3, 2020 01:11
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 2, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 2, 2020
@mvladev
Copy link
Author

mvladev commented Nov 2, 2020

This should ideally be the general pattern for most controllers that have resource definitions, right?

Yes. This is the idea.

@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 6, 2020
@amshuman-kr
Copy link
Collaborator

@mvladev Sorry for the delay in review. There is an upcoming release this week for etcd-backup-restore and etcd-druid and we want to pick this after that release.

@gardener-robot
Copy link

@mvladev You need rebase this pull request with latest master branch. Please check.

@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Apr 26, 2021
@amshuman-kr amshuman-kr mentioned this pull request May 4, 2021
Merged
@amshuman-kr
Copy link
Collaborator

@mvladev To avoid dealing with the conflicts in vendoring during rebase, I have cherry-picked that last two commits from this PR in a fresh PR #169. Shall I close this PR in favour of that?

@gardener-robot gardener-robot removed the needs/rebase Needs git rebase label May 10, 2021
@amshuman-kr
Copy link
Collaborator

/close via #169

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/second-opinion Needs second review by someone else size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mvladev @amshuman-kr @gardener-robot @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3