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

Implement new echo behavior and command #139

Merged
merged 9 commits into from Oct 25, 2019
Merged

Implement new echo behavior and command #139

merged 9 commits into from Oct 25, 2019

Conversation

juliobbv
Copy link
Contributor

@juliobbv juliobbv commented Oct 22, 2019 •
edited

Command echoing as a default behavior tends to clutter the user logs, so we want to swap to a system where users have to opt in to see this information.

Command outputs will still be echoed in the case there are any errors processing such commands. This is so the end user can have more context on why the command failed and help with troubleshooting.

Echo output in the user logs can be explicitly controlled by the new commands ::echo::on and ::echo::off. By default, echoing is enabled if ACTIONS_STEP_DEBUG secret is enabled, otherwise echoing is disabled.

ToDo

  • Test pass with new ::echo::on and ::echo::off action commands
  • Setting ACTIONS_STEP_DEBUG secret sets echo on by default
  • L0 tests

@thboop
Copy link
Collaborator

thboop commented Oct 22, 2019

Can we link the adr/issue?

@thboop
Copy link
Collaborator

thboop commented Oct 22, 2019

Lets make sure we have a ticket or pr open to update the toolkit

@TingluoHuang
Copy link
Member

Normally, when system.debug is on, all extra log will start with ##debug

Looks like the output produced here won’t starts with ##debug

An ADR would help😀

@juliobbv
Copy link
Contributor Author

@TingluoHuang BTW, here's the ADR: https://github.com/github/pe-actions-runtime/pull/136.

@thboop, @bryanmacfarlane, and I had a conversation on the ##debug thing. We decided to direct the command processing to the regular output logs, even when debug is on. This is for logging consistency -- with debug off, we can output error for only the command processing failures to output/error streams respectively, and with debug on, the successfully-processed commands go to output as well.

@juliobbv
Copy link
Contributor Author

juliobbv commented Oct 22, 2019 •
edited

@thboop I don't think we can easily link to the ADR, as it lives in a private repo, and this repo will eventually be open-sourced.

I copied most of the ADR contents to this PR description as a mitigation.

@juliobbv juliobbv force-pushed the users/juliobbv/newEchoBehavior branch 2 times, most recently from abc2533 to 4a3c94f Compare October 22, 2019 21:31
@juliobbv juliobbv force-pushed the users/juliobbv/newEchoBehavior branch from 4a3c94f to 38f4154 Compare October 23, 2019 14:02
@juliobbv juliobbv force-pushed the users/juliobbv/newEchoBehavior branch from 06744c8 to 0a7cefd Compare October 23, 2019 18:23
@juliobbv juliobbv changed the title Implement new echo behavior and commands. Implement new echo behavior and command Oct 23, 2019
@juliobbv juliobbv force-pushed the users/juliobbv/newEchoBehavior branch from 0a7cefd to 09e5853 Compare October 23, 2019 18:24
@juliobbv juliobbv force-pushed the users/juliobbv/newEchoBehavior branch from 09e5853 to e815969 Compare October 23, 2019 18:26
@juliobbv
Copy link
Contributor Author

Toolkit issue to leverage the echo command: actions/toolkit#192

cc @thboop

thboop
thboop approved these changes Oct 24, 2019
View reviewed changes
Copy link
Collaborator

@thboop thboop left a comment

Choose a reason for hiding this comment

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

Minor thoughts, LGTM

@juliobbv juliobbv force-pushed the users/juliobbv/newEchoBehavior branch from f3855be to 4ced8a1 Compare October 24, 2019 18:17
@thboop
Copy link
Collaborator

thboop commented Oct 24, 2019

Updates look good

@thboop
Copy link
Collaborator

thboop commented Oct 24, 2019 •
edited

Going to merge this tomorrow if thats cool with you @ericsciple.

@thboop thboop merged commit 82e9857 into master Oct 25, 2019
@thboop thboop deleted the users/juliobbv/newEchoBehavior branch October 25, 2019 14:39
AdamOlech pushed a commit to antmicro/runner that referenced this pull request Jan 28, 2021
@juliobbv @thboop
Implement new echo behavior and command (actions#139)
66ca7ac 
* Remove controlling echoing by command

* Add 'echo on' and 'echo off' action commands

* PR feedback and add L0 tests

* Register new command

* Eric's PR feedback

* Tweak logging a bit

* Rename EchoOnActionCommandSuccess -> EchoOnActionCommand

* More PR reaction

* Make warning messages in Action Commands not rely on context from echo commands
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericsciple ericsciple ericsciple left review comments

@thboop thboop thboop approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@juliobbv @thboop @TingluoHuang @ericsciple