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

Initialize cobra-cli repo #1

Merged
merged 4 commits into from Feb 17, 2022
Merged

Initialize cobra-cli repo #1

merged 4 commits into from Feb 17, 2022

Conversation

liggitt
Copy link
Collaborator

@liggitt liggitt commented Feb 10, 2022

This PR sets up various housekeeping things for the new repo:

  • Copies existing license/contributing/conduct/CI root files from spf13/cobra
  • Adds copies of make targets from spf13/cobra, adjust CI config
  • Rename packages and go.mod to cobra-cli
  • Tidy go module

Because this repo placed the binary at the root, it makes the binary name change to cobra-cli. Before merging any changes into this repo, we should make sure that's what we want, since that ripples to docs and usage, not just the install command (asked in spf13/cobra#1597 (comment))

If we want to preserve the cobra binary, we should reinitialize it with the history placed under a github.com/spf13/cobra-cli/cobra subpackage.

Preview of the configured CI runs passing at https://github.com/liggitt/cobra-cli/actions?query=branch%3Ainit

Signed-off-by: Jordan Liggitt <liggitt@google.com>
Signed-off-by: Jordan Liggitt <liggitt@google.com>
Signed-off-by: Jordan Liggitt <liggitt@google.com>
Signed-off-by: Jordan Liggitt <liggitt@google.com>
@spf13
Copy link
Owner

spf13 commented Feb 11, 2022

@liggitt This looks good, but I'm going to wait for a bit on merging it until I can resolve my concern with DCOBot. There's a thread on the Cobra project about using it over CLA Assistant and I thought I'd try it here first.

In reviewing it I had a concern about how the signing happens on the commit without every acknowledging the agreement (unlike CLA Assistant where you have to visually see the agreement and click "I Agree"). I brought this up with a lawyer and they shared my concern. In legal terms this is an inferred agreement, and typically very weak. It's well established that clicking I accept connotes acceptance, but it's not clear that signing a commit has any legal precedence yet.

I'm going to bring this up with someone with deeper expertise in the legal field of open source and see if I can get more guidance. If this can wait for a few days that would be best.

@liggitt
Copy link
Collaborator Author

liggitt commented Feb 11, 2022

ack

@liggitt liggitt marked this pull request as ready for review February 11, 2022 18:26
@liggitt
Copy link
Collaborator Author

liggitt commented Feb 11, 2022

sounds like the consensus was to keep the cobra-cli name, so this is ready, content-wise. moving out of draft, but will hold for merge until @spf13 gets resolution on the DCO/CLA thing.

Copy link
Collaborator

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

Also looks good to me!

After this merges and the dust has settled a bit, we can touch up the readme, add labels to this repo, and do other maintence bits as needed

@liggitt
Copy link
Collaborator Author

liggitt commented Feb 17, 2022

I'm going to bring this up with someone with deeper expertise in the legal field of open source and see if I can get more guidance. If this can wait for a few days that would be best.

@spf13, any update on this bit?

@spf13
Copy link
Owner

spf13 commented Feb 17, 2022 via email

@spf13
Copy link
Owner

spf13 commented Feb 17, 2022

I have an update which you can read spf13/cobra#1555 (comment)

As a result of these findings, I'll go ahead and setup CLA Assistant on this repo too.

@spf13
Copy link
Owner

spf13 commented Feb 17, 2022

I've setup CLA Assistant but I believe that because this PR predated it it's not being required. It might require to resubmit the PR to trigger it. @liggitt would that be too much trouble. I'm not sure what alternatives we have.

@liggitt liggitt closed this Feb 17, 2022
@liggitt liggitt reopened this Feb 17, 2022
@CLAassistant
Copy link

CLAassistant commented Feb 17, 2022

CLA assistant check
All committers have signed the CLA.

@spf13
Copy link
Owner

spf13 commented Feb 17, 2022

That worked!! Thanks @liggitt

@liggitt liggitt merged commit 7c6e26a into spf13:main Feb 17, 2022
- 14
- 15
- 16
- 17
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend that you also include Windows in the matrix and reduce the numbers by cutting one or two Go versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We do test against windows here:

And yes, will remove Go 14 and 15.

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