-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: builds namespace #620
Conversation
@vladfrangu I've manually validated all the functionality provided by this PR, and it works as expected. Thank you! I'd wait with merging for the Cucumber spec, though. Just one improvement proposal: |
I've just noticed the
|
Thats what it does @netmilk, re Related to |
Understood. Perfect. Thank you for explaining it to me. What about |
So what would --log do? Attach the log stream on command run? And wdym by attach / detach for streaming? |
@vladfrangu, I'm sorry for the ambiguity. Yes, it would connect By "streaming" I mean connecting to platform live events during a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, but we need at least the cucumber tests before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few copy nits I noticed when skimming the PR... the screenshots look bomb!
Btw do we communicate that "Builds" are a platform-only feature anywhere? As a new user, I would definitely assume that I can build the Actors locally with CLI as well (maybe more of a question for @netmilk , sorry if this has been discussed somewhere before).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that your run
methods tend to be very long, which makes them hard to read. Other than that and a few minor comments/questions, I think this is good to go.
as discussed on slack, i would love to see more e2e test assertions, i get that it might be too complicated to deal with it properly, but at least some lower effort asserts (e.g. to detect one line we know should be there) would be great |
Screenshots incoming, tests too (probably will rely on cucumber tests)
Closes part of the #619 epic
builds ls
builds info
builds info --json
builds create --actor=id
builds create --actor=id --tag=string | multiple versions tagged the same way
builds create --actor=id --tag=non-existent
THIS NEEDS A REVIEW FROM @netmilk!!
builds create --actor=id --tag=string --version=string