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

workflows: add self-hosted runners #50468

Merged
merged 1 commit into from
Feb 23, 2020
Merged

workflows: add self-hosted runners #50468

merged 1 commit into from
Feb 23, 2020

Conversation

jonchang
Copy link
Contributor

No description provided.

@request-info request-info bot added the needs response Needs a response from the issue/PR author label Feb 20, 2020
@Homebrew Homebrew deleted a comment from request-info bot Feb 20, 2020
@jonchang jonchang removed the needs response Needs a response from the issue/PR author label Feb 20, 2020
@jonchang jonchang changed the title Testing self-hosted runners workflows: add self-hosted runners Feb 23, 2020
@jonchang jonchang merged commit d0a1667 into Homebrew:master Feb 23, 2020
@jonchang jonchang deleted the testing-runner branch February 23, 2020 16:21
brew uninstall --force --ignore-dependencies $BREW_LIST
fi
rm -rf bottles
rm -rf *
Copy link
Member

Choose a reason for hiding this comment

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

What's it cleaning up here? Does GitHub Actions not provide a clean space to each runner?

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest this cleanup logic go into brew test-bot, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does GitHub Actions not provide a clean space to each runner?

For GitHub hosted runners, yes, but this is basically like Jenkins where we need to do all of that.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

run: |
mkdir bottles
cd bottles
brew test-bot --ci-pr
Copy link
Member

Choose a reason for hiding this comment

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

--ci-pr should be autodetected from the environment variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This copies the Jenkins configuration.

Copy link
Member

Choose a reason for hiding this comment

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

Think it could be removed there too!

HOMEBREW_NO_ANALYTICS: 1
HOMEBREW_NO_AUTO_UPDATE: 1
steps:
- name: Setup tap
Copy link
Member

Choose a reason for hiding this comment

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

Could this use actions/checkout@master?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This copies the Jenkins configuration. I believe test bot looks at the current brew repository, not its current directory.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this, can you elaborate a bit?

BREW_LIST=$(brew list 2>/dev/null)
if [ -n "$BREW_LIST" ]; then
echo ">> brew uninstall"
brew uninstall --force --ignore-dependencies $BREW_LIST
Copy link
Member

Choose a reason for hiding this comment

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

brew test-bot should handle this already. If it doesn't: I suggest it's added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn’t. I need to figure out what environment variables test bot is looking at on Jenkins to determine its cleanup behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, thanks 👍

BREW_LIST=$(brew list 2>/dev/null)
if [ -n "$BREW_LIST" ]; then
echo ">> brew uninstall"
brew uninstall --force --ignore-dependencies $BREW_LIST
Copy link
Member

Choose a reason for hiding this comment

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

brew test-bot should definitely handle this already.

brew uninstall --force --ignore-dependencies $BREW_LIST
fi
rm -rf bottles
rm -rf *
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest this cleanup logic go into brew test-bot, too.

@lock lock bot added the outdated PR was locked due to age label Mar 27, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants