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

feat: builds namespace #620

Merged
merged 27 commits into from
Sep 9, 2024
Merged

feat: builds namespace #620

merged 27 commits into from
Sep 9, 2024

Conversation

vladfrangu
Copy link
Member

@vladfrangu vladfrangu commented Aug 26, 2024

Screenshots incoming, tests too (probably will rely on cucumber tests)

Closes part of the #619 epic

builds ls

Code - 2024-08-26 at 19 49 20

builds info

Code - 2024-08-26 at 19 49 53

builds info --json

Code - 2024-08-26 at 19 51 30

builds create --actor=id

Code - 2024-08-26 at 20 33 22

builds create --actor=id --tag=string | multiple versions tagged the same way

Code - 2024-08-26 at 20 34 22

builds create --actor=id --tag=non-existent

THIS NEEDS A REVIEW FROM @netmilk!!

Code - 2024-08-26 at 20 35 11

builds create --actor=id --tag=string --version=string

Code - 2024-08-26 at 20 35 37

@vladfrangu vladfrangu added the adhoc Ad-hoc unplanned task added during the sprint. label Aug 26, 2024
@vladfrangu vladfrangu requested review from netmilk and B4nan August 26, 2024 16:48
@vladfrangu vladfrangu self-assigned this Aug 26, 2024
@github-actions github-actions bot added this to the 97th sprint - Tooling team milestone Aug 26, 2024
@github-actions github-actions bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Aug 26, 2024
@vladfrangu vladfrangu marked this pull request as ready for review August 27, 2024 14:43
@netmilk
Copy link
Contributor

netmilk commented Aug 30, 2024

@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:
Shouldn't the $ apify build create always attach to the log the same way apify push does? If so, I'd add --detach flag to exit 0 right after successfully creating a build.

@netmilk
Copy link
Contributor

netmilk commented Aug 30, 2024

I've just noticed the $ apify builld log subcommand, which is implemented and isn't mentioned in this PR. Because of that I've missed it initially.

$ apify build log for a running build should attach terminal to the build session and keep printing until the build is finished.

@vladfrangu
Copy link
Member Author

Thats what it does @netmilk, re apify build log 👀

Related to apify builds create, do we.. want to also attach the logs automatically? Can we get that documented on notion too (also maybe make it opt-in, or bring in the --wait-for-finish flag from other cmds)

@netmilk
Copy link
Contributor

netmilk commented Aug 30, 2024

Understood. Perfect. Thank you for explaining it to me.

What about $ apify builds create --log and let's keep attach / dettach words for the streaming?

@vladfrangu
Copy link
Member Author

So what would --log do? Attach the log stream on command run? And wdym by attach / detach for streaming?

@netmilk
Copy link
Contributor

netmilk commented Aug 30, 2024

@vladfrangu, I'm sorry for the ambiguity. Yes, it would connect apify build log to the newly created build.

By "streaming" I mean connecting to platform live events during a run. Run, Dataset, KVS, RQ Creation, Status messages, push-data, set-value as briefly mentioned in the internal spec.

Copy link
Member

@B4nan B4nan left a 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

src/lib/commands/pretty-print-bytes.ts Show resolved Hide resolved
src/lib/commands/pretty-print-status.ts Outdated Show resolved Hide resolved
src/commands/builds/ls.ts Outdated Show resolved Hide resolved
@vladfrangu vladfrangu requested a review from B4nan September 2, 2024 16:42
Copy link
Contributor

@barjin barjin left a 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).

src/commands/builds/create.ts Outdated Show resolved Hide resolved
src/commands/builds/create.ts Outdated Show resolved Hide resolved
src/commands/builds/create.ts Outdated Show resolved Hide resolved
src/commands/builds/info.ts Outdated Show resolved Hide resolved
src/commands/builds/info.ts Outdated Show resolved Hide resolved
src/commands/builds/log.ts Outdated Show resolved Hide resolved
src/commands/builds/log.ts Outdated Show resolved Hide resolved
src/commands/builds/log.ts Outdated Show resolved Hide resolved
src/commands/builds/ls.ts Outdated Show resolved Hide resolved
src/commands/builds/ls.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@janbuchar janbuchar left a 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.

.github/workflows/cucumber.yaml Show resolved Hide resolved
src/commands/builds/ls.ts Show resolved Hide resolved
src/commands/builds/info.ts Outdated Show resolved Hide resolved
src/commands/builds/info.ts Outdated Show resolved Hide resolved
@vladfrangu vladfrangu mentioned this pull request Sep 9, 2024
@B4nan
Copy link
Member

B4nan commented Sep 9, 2024

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

@vladfrangu vladfrangu merged commit 6345162 into master Sep 9, 2024
21 checks passed
@vladfrangu vladfrangu deleted the feat/builds-namespace branch September 9, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants