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.

Sign up for GitHub

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

Jump to bottom

Allow task log prefixing in Github Actions #5994

Merged
merged 5 commits into from
Sep 22, 2023
Merged

Allow task log prefixing in Github Actions #5994

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7d89e86
Go side: add task prefixing in Github actions
Zertsov Sep 21, 2023
416d0b9
add Rust fix for gh action log prefix
Zertsov Sep 21, 2023
b67a7e4
move github action env var to each test and filter flag to task test
Zertsov Sep 21, 2023
1bf87a2
Merge branch 'main' into voz/gh-actions-log-prefix
Zertsov Sep 21, 2023
b2073bf
Merge branch 'main' into voz/gh-actions-log-prefix
Zertsov Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions cli/internal/run/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,11 @@ func optsFromArgs(args *turbostate.ParsedArgsFromRust) (*Opts, error) {
}

func configureRun(base *cmdutil.CmdBase, opts *Opts, signalWatcher *signals.Watcher) *run {
// TODO: We currently don't respect the user's input if they ask for task prefixes on
// GitHub Actions and forcibly strip tasks from their logs even if they specify
// that they want it. This should be fixed in Go and Rust at the same time.
if opts.runOpts.LogOrder == "auto" && ci.Constant() == "GITHUB_ACTIONS" {
opts.runOpts.LogOrder = "grouped"
opts.runOpts.LogPrefix = "none"
if opts.runOpts.LogPrefix != "task" {
opts.runOpts.LogPrefix = "none"
}
opts.runOpts.IsGithubActions = true
}

Expand Down
15 changes: 9 additions & 6 deletions crates/turborepo-lib/src/opts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
pub struct RunOpts<'a> {
pub(crate) tasks: &'a [String],
pub(crate) concurrency: u32,
parallel: bool,

Check warning on line 60 in crates/turborepo-lib/src/opts.rs

View workflow job for this annotation

GitHub Actions / Build Turborepo (ubuntu, ubuntu-latest) 

fields `parallel`, `profile`, `continue_on_error`, `dry_run`, and `summarize` are never read

Check warning on line 60 in crates/turborepo-lib/src/opts.rs

View workflow job for this annotation

GitHub Actions / Build Turborepo (macos, macos-latest) 

fields `parallel`, `profile`, `continue_on_error`, `dry_run`, and `summarize` are never read
pub(crate) env_mode: EnvMode,
// Whether or not to infer the framework for each workspace.
pub(crate) framework_inference: bool,
Expand Down Expand Up @@ -114,12 +114,15 @@
});

let (is_github_actions, log_order, log_prefix) = match args.log_order {
// TODO: We currently don't respect the user's input if they ask for task prefixes on
// GitHub Actions and forcibly strip tasks from their logs even if they specify
// that they want it. This should be fixed in Go and Rust at the same time.
LogOrder::Auto if turborepo_ci::Vendor::get_constant() == Some("GITHUB_ACTIONS") => {
(true, ResolvedLogOrder::Grouped, ResolvedLogPrefix::None)
}
LogOrder::Auto if turborepo_ci::Vendor::get_constant() == Some("GITHUB_ACTIONS") => (
true,
ResolvedLogOrder::Grouped,
match args.log_prefix {
LogPrefix::Task => ResolvedLogPrefix::Task,
_ => ResolvedLogPrefix::None,
},
),

// Streaming is the default behavior except when running on GitHub Actions
LogOrder::Auto | LogOrder::Stream => {
(false, ResolvedLogOrder::Stream, args.log_prefix.into())
Expand Down
30 changes: 30 additions & 0 deletions turborepo-tests/integration/tests/ordered/github.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,36 @@ because otherwise prysk interprets them as multiline commands
Cached: 0 cached, 2 total
Time:\s*[\.0-9]+m?s (re)

# Build as if we are in Github Actions with a task log prefix.
$ export GITHUB_ACTIONS=1
Copy link
Contributor

Choose a reason for hiding this comment

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

We can drop this since it's set on line 8

Suggested change
$ export GITHUB_ACTIONS=1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably my unfamiliarity with the .t files, but I thought that that export was just for the test it was running in. Can any single test do changes to subsequent tests (in this case, doing the export affecting all other tests)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Prysk runs each file in a new shell, so they're not localized per-test. The way these comparisons work it:

  • Ignores all unindented lines.
  • Runs all of the $-prefixed ones.
  • inlines stdout + stderr
  • Grabs the output contents of the entire shell.
  • Checks for pass fail based on diff of the input and the output.

If you ever get a failing test it pops out a .t.err file which you can just diff with the .t file. It's ... primitive? But has saved us enough times to be worth dealing with bad regex ergonomics, a Python toolchain, and other quirks.

$ ${TURBO} run build --force --log-prefix="task"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$ export GITHUB_ACTIONS=1
$ ${TURBO} run build --force --log-prefix="task"
GITHUB_ACTIONS=1 ${TURBO} run build --force --log-prefix="task" --filter=util
  • not blocking, but i try to set env vars just for the command so it doesn't leak into other tests in the file
  • added a filter flag so you don't run into non-deterministic ordering output. Based on the fact that util#build is logged after my-app#build in this test right now, I'm guessing they are running concurrently

Copy link
Contributor Author

@Zertsov Zertsov Sep 21, 2023 •
edited

Choose a reason for hiding this comment

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

Should I apply this to the preceding test? Also if I'm not mistaken don't these .t files need to start with a $ in order for the command to be executed in a shell?

Another thing: by passing the filter it won't run the two tasks - do we care to have two tasks or is it ok to just test that a single task has the prefix?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to the --filter

Copy link
Contributor

@mehulkar mehulkar Sep 22, 2023 •
edited

Choose a reason for hiding this comment

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

do we care to have two tasks or is it ok to just test that a single task has the prefix?

single task is fine IMO

Should I apply this to the preceding test?

generally, I wouldn't change unrelated, existing code. That has a tendency to balloon the diff and make git history harder to follow (not to mention it might cause unexpected issues and sink your time). In this case, 🤷🏾

\xe2\x80\xa2 Packages in scope: my-app, util (esc)
\xe2\x80\xa2 Running build in 2 packages (esc)
\xe2\x80\xa2 Remote caching disabled (esc)
::group::my-app:build
my-app:build: cache bypass, force executing 4c3a4e8d472d74b2
my-app:build:
my-app:build: > build
my-app:build: > echo 'building' && sleep 1 && echo 'done'
my-app:build:
my-app:build: building
my-app:build: done
::endgroup::
::group::util:build
util:build: cache bypass, force executing 90d7154e362e3386
util:build:
util:build: > build
util:build: > sleep 0.5 && echo 'building' && sleep 1 && echo 'completed'
util:build:
util:build: building
util:build: completed
::endgroup::

Tasks: 2 successful, 2 total
Cached: 0 cached, 2 total
Time:\s*[\.0-9]+m?s (re)


Verify that errors are grouped properly
$ ${TURBO} run fail
\xe2\x80\xa2 Packages in scope: my-app, util (esc)
Expand Down