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

[EP] CLI #432

Merged
merged 2 commits into from
Dec 7, 2020
Merged

[EP] CLI #432

merged 2 commits into from
Dec 7, 2020

Conversation

otaviof
Copy link
Member

@otaviof otaviof commented Oct 7, 2020

Enhancement proposal for creating a command-line interface for Shipwright's Build. A proof-of-concept implementation can be found on otaviof/shp.

Linked with issue #382, creates the foundation for #59.

Preview Videos

Copy link
Member

@gabemontero gabemontero left a comment

Choose a reason for hiding this comment

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

First pass set of comments @otaviof


### Standalone

The standalone binary will be available via the usual means, final users will be able to simply
Copy link
Member

Choose a reason for hiding this comment

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

Some elaboration here would be good... minimally, point to the Releasing section for both short term and long term goals.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'm adding a pointer to "releasing".


Will be responsible for managing Build objects, using the verbs: `create`, `update`, list and
`delete`. End users will have the ability to bootstrap a repository container image build with a
single command, similarly to `shp init`, but with no assumptions about local project folder.
Copy link
Member

Choose a reason for hiding this comment

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

Granted, this is upstream and we don't want to necessarily proscribe an overly openshift slant to things, but we have resounding, positive, feedback with the oc new-app and oc new-build experience of pointing at a repo or local file system with constructing build related artifacts, that we need to at least consider it. Even if we at least initially rule it out, but consider for later.

It would certainly tie in with your associated local/remote EPs.

So minimally touching upon the correlation here seems prudent.

Copy link
Member Author

Choose a reason for hiding this comment

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

The shp init should take place in a future EP, so I'm redacting that part from this EP, and mentioning the correlation with with other features, that's indeed prudent. Please consider latest commit.

Copy link
Contributor

@HeavyWombat HeavyWombat left a comment

Choose a reason for hiding this comment

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

Looks super good. Found just one minor TYPO. One more thing maybe: Regarding the name shp, I do not really have a good alternative in my head, but I think shp carries to few context to help correlate it with the build project here.

docs/proposals/cli.md Outdated Show resolved Hide resolved

## Command

The project will use [cobra](https://github.com/spf13/cobra) for scaffolding the command-line
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well include Viper also https://github.com/spf13/viper for saving configuration files etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. Viper comes handy for this type of command-line together with Cobra. Thanks.

@qu1queee
Copy link
Contributor

is this blocked somehow? otherwise we should push on this one, CLI is pretty important for reaching wider adoption of Build.

@otaviof
Copy link
Member Author

otaviof commented Nov 23, 2020

is this blocked somehow? otherwise we should push on this one, CLI is pretty important for reaching wider adoption of Build.

I will go through the comments and make sure it's up-to-speed for today's community meeting.

Copy link
Contributor

@HeavyWombat HeavyWombat left a comment

Choose a reason for hiding this comment

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

Really like it! Thanks for the proposal. Would be very happy to help. Just added one question with regards to the usage of bats. Otherwise I have nothing to add.

Comment on lines 167 to 182
## Testing

The strategy to test the command-line application will be based on `go test` for the unit testing,
and for end-to-end testing we should adopt [bats](https://github.com/sstephenson/bats). Bats
framework gives us a structured way to run shell script commands and what we expect them to
return, the command-line client shp is no more than a shell command.

The testing structure will be composed of:

- **Unit**: written in Golang and using [Gomega](https://onsi.github.io/gomega/) for assertion;
- **End-to-End**: written in Golang and located at the traditional `test/e2e` directory;
- **System**: written using Bats framework (Bash);

Bats will also be helpful for a future `shp` container-image, we are able to mount the Bats files
in the container-image produced, and run our system testing against it. Therefore, Bats covers
testing of a local command-line binary, as well as it does a container-image.
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick question regarding the proposed tools: I have worked with both bats and Ginkgo. Definitely good way to do it. However, I never really made friends with bats, especially when trying to debug an issue and I was missing some ad-hoc flexibility. For one of our tools, I adapted a style I saw in the open-source tool spruce. A helper function in the test suite works as a stand-in for the binary and you keep writing all your tests in Go while the test definition looks kind of like your command line would. Maybe you can have a look at the idea.

Copy link
Member Author

@otaviof otaviof Nov 25, 2020

Choose a reason for hiding this comment

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

Taking your experience in consideration I'm rewording to make Bats optional, we may decide for another tool depending on the group experience with it. I think it's fair to give it a try, follow buildah's example.

Thanks for sharing more options for this role.

Copy link
Contributor

Choose a reason for hiding this comment

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

The buildah example looks nice. Something like that could be fun. Looking forward to see this in action.

@otaviof otaviof requested a review from HeavyWombat November 25, 2020 17:52
Copy link
Contributor

@HeavyWombat HeavyWombat left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2020
@qu1queee qu1queee self-requested a review November 26, 2020 08:13
@qu1queee
Copy link
Contributor

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2020
$ kubectl shp ...
```

And the following global parameters are expected, to keep consistency with `kubectl`:
Copy link
Contributor

Choose a reason for hiding this comment

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

s/parameters/flags/

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

docs/proposals/cli.md Outdated Show resolved Hide resolved
$ shp <resource> <verb> <name> [options]
```

For example:
Copy link
Contributor

Choose a reason for hiding this comment

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

The examples below do not follow the above mentioned shp <resource> <verb> syntax.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated them.

$ shp logs nodejs-ex
```

During the review process of this enhancement proposal, we must decide which pattern suits better,
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for the shp <verb> <resource> pattern


The testing structure will be composed of:

- **Unit**: written in Golang and using [Gomega](https://onsi.github.io/gomega/) for assertion;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this belongs here, but I would prefer to forgo using Gomega and stick with pure go tests.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2020
@qu1queee
Copy link
Contributor

qu1queee commented Dec 4, 2020

@otaviof We fixed an issue on CI, maybe you can rebase on top of master

@otaviof otaviof dismissed gabemontero’s stale review December 6, 2020 11:16

Changes were applied accordingly.

@qu1queee
Copy link
Contributor

qu1queee commented Dec 7, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2020
@qu1queee
Copy link
Contributor

qu1queee commented Dec 7, 2020

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HeavyWombat, qu1queee

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 206fcb1 into shipwright-io:master Dec 7, 2020
@otaviof otaviof deleted the ep-cli branch February 19, 2021 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants