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

Allow send SSH into runner info. to DM #33346

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Allow send SSH into runner info. to DM #33346

merged 1 commit into from
Sep 12, 2024

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Sep 6, 2024

What does this PR do?

Easier to get notified.

#because the SSH can be enabled dynamically if the workflow failed, so we need to store slack infos to be able to retrieve them during the waitforssh step
shell: bash
run: |
if [ "${{ secrets[format('{0}_{1}', github.actor, 'SLACK_ID')] }}" != "" ]; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If a GitHub secret is added to the repository: for example, YDSHIEH_SLACK_ID with a value being the corresponding slack user ID

Copy link
Collaborator

Choose a reason for hiding this comment

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

Quick Q, should the slack id not be an argument to workflow_dispatch 🤗

Copy link
Collaborator Author

@ydshieh ydshieh Sep 6, 2024

Choose a reason for hiding this comment

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

It is an ID like X13JMGOCTSU, and has to be copied from the Slack user profile.
Being an input might add some friction to this process (but browser usually remember input)

The more IMPORTANT point is: we want to keep it as a secret (and not show on log), like what we did so far for

${{ secrets.SLACK_CIFEEDBACK_CHANNEL }}

@ydshieh ydshieh requested a review from amyeroberts September 6, 2024 10:15
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@ydshieh ydshieh requested review from ArthurZucker and removed request for amyeroberts and ArthurZucker September 6, 2024 12:43
if [ "${{ secrets[format('{0}_{1}', github.actor, 'SLACK_ID')] }}" != "" ]; then
echo "SLACKCHANNEL=${{ secrets[format('{0}_{1}', github.actor, 'SLACK_ID')] }}" >> $GITHUB_ENV
else
echo "SLACKCHANNEL=${{ secrets.SLACK_CIFEEDBACK_CHANNEL }}" >> $GITHUB_ENV
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we're adding this to the GITHUB_ENV, is this less secure? Previously the information was stored in secrets - will making it persistent like this make it more accessible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it would still be masked by GitHub Actions.

At least, it follows what has been done by Guillaume.

Copy link
Contributor

Choose a reason for hiding this comment

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

we need to test. normally github keep it obfuscated on the logs.
if it's not the case, eventually we can add this kind of command : echo "::add-mask::$the_secret"

Copy link
Contributor

Choose a reason for hiding this comment

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

let's try this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI: there is a run page

https://github.com/huggingface/transformers/actions/runs/10736292447/job/29775922690

In its raw log page, searching SLACKCHANNEL, all places are masked ***

@ydshieh
Copy link
Collaborator Author

ydshieh commented Sep 11, 2024

@ArthurZucker @LysandreJik

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Cool ! Can you have @glegendre01 review this just for security purposes?

@ydshieh
Copy link
Collaborator Author

ydshieh commented Sep 12, 2024

Thanks for the review. Sure I will ping him.

@ydshieh ydshieh requested a review from glegendre01 September 12, 2024 12:29
if [ "${{ secrets[format('{0}_{1}', github.actor, 'SLACK_ID')] }}" != "" ]; then
echo "SLACKCHANNEL=${{ secrets[format('{0}_{1}', github.actor, 'SLACK_ID')] }}" >> $GITHUB_ENV
else
echo "SLACKCHANNEL=${{ secrets.SLACK_CIFEEDBACK_CHANNEL }}" >> $GITHUB_ENV
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to test. normally github keep it obfuscated on the logs.
if it's not the case, eventually we can add this kind of command : echo "::add-mask::$the_secret"

if [ "${{ secrets[format('{0}_{1}', github.actor, 'SLACK_ID')] }}" != "" ]; then
echo "SLACKCHANNEL=${{ secrets[format('{0}_{1}', github.actor, 'SLACK_ID')] }}" >> $GITHUB_ENV
else
echo "SLACKCHANNEL=${{ secrets.SLACK_CIFEEDBACK_CHANNEL }}" >> $GITHUB_ENV
Copy link
Contributor

Choose a reason for hiding this comment

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

let's try this PR

@ydshieh ydshieh merged commit e688996 into main Sep 12, 2024
9 checks passed
@ydshieh ydshieh deleted the ssh_send_dm branch September 12, 2024 14:03
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
allow send DM

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants