Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

cmd/operator-sdk/new: make git init opt-in with --git-init flag #1588

Merged
merged 5 commits into from
Jun 28, 2019

Conversation

hasbro17
Copy link
Contributor

@hasbro17 hasbro17 commented Jun 24, 2019

Description of the change:
Changed the flag --skip-git-init to --git-init to make git init opt-in instead of opt-out.
Also operator-sdk new will no longer create the initial commit.

Motivation for the change:
Creating the initial commit is unexpected behavior and can fail certain scenarios as described in #1198 .
Having git init as opt-out via --skip-git-init instead of opt-in is debatable, and so is unchanged at the moment.
I am leaning towards making git init opt-in but can address that in a follow up PR after we discuss it on the issue.
Changed git init to opt-in per the consensus from the community and maintainers.

Closes: #1198

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 24, 2019
@hasbro17 hasbro17 requested review from estroz, lilic and AlexNPavel June 24, 2019 03:10
Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

IIRC we did a poll on slack and we said lets skip git init altogether? And make it opt in instead. If my memory serves me right.

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/approve

@hasbro17
Copy link
Contributor Author

/hold

After further discussion, we're going to make git init opt-in instead of being the default.

@hasbro17 hasbro17 changed the title cmd/operator-sdk/new: don't commit after git init [WIP] cmd/operator-sdk/new: don't commit after git init Jun 24, 2019
@joelanford joelanford self-requested a review June 25, 2019 03:18
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2019
@hasbro17 hasbro17 force-pushed the remove-git-commit branch from 4af5af6 to 31c242c Compare June 26, 2019 19:28
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2019
@hasbro17 hasbro17 changed the title [WIP] cmd/operator-sdk/new: don't commit after git init cmd/operator-sdk/new: don't commit after git init Jun 26, 2019
@hasbro17 hasbro17 requested a review from lilic June 26, 2019 19:33
@hasbro17
Copy link
Contributor Author

@joelanford @lilic PTAL again.

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Few comments but overall lgtm

We should update the migration guide as well, that's the only other place I found this flag mentioned -> https://github.com/operator-framework/operator-sdk/blob/2ad2ee217424fcfde9bb4cfba00d845f988ec511/doc/migration/v0.1.0-migration-guide.md#create-a-new-v010-project

@hasbro17
Copy link
Contributor Author

We should update the migration guide as well, that's the only other place I found this flag mentioned

@lilic That migration guide is for v0.0.7=>v0.1.0 both of which had the --skip-git-init flag so it makes sense to keep that in the guide for users who still need to migrate from pre-controller-runtime projects (if there are still any left).

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

One minor nit, otherwise

/approve

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 27, 2019
@hasbro17 hasbro17 changed the title cmd/operator-sdk/new: don't commit after git init cmd/operator-sdk/new: make git init opt-in with --git-init flag Jun 27, 2019
Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@hasbro17 hasbro17 merged commit eedbe31 into operator-framework:master Jun 28, 2019
@hasbro17 hasbro17 deleted the remove-git-commit branch June 28, 2019 18:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

operator-sdk new should not git commit
5 participants