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

ci: adopt asdf for internal dev. init WSL test environments #956

Merged
merged 40 commits into from May 26, 2021
Merged

ci: adopt asdf for internal dev. init WSL test environments #956

merged 40 commits into from May 26, 2021

Conversation

jthegedus
Copy link
Contributor

@jthegedus jthegedus commented May 25, 2021

Summary

Adopt asdf for out tests, internal dev. Init WSL test envs. Cleanup Vagrantfile.

  • remove unused Vagrantfile
  • provide .tool-versions for asdf development and CI workflows
  • extract Lint and Format GitHub Workflows and use .tool-versions with asdf/actions
    • remove lint.sh as our CI pipeline now enforces all formatting and linting reliably, so no need to run this script manually before release, nor for it to exist outside the CI Workflow definition.
  • extract Bats testing to individual jobs per OS type.
    • nix:
      • install test deps as before
      • run on each of macos-10.15, macos-11 (disabled as it's in preview), ubuntu-18.04 & ubuntu-20.04
    • wsl1:

@jthegedus jthegedus requested a review from a team as a code owner May 25, 2021 07:41
@jthegedus jthegedus changed the title ci: adopt asdf for internal dev. init Windows WSL test environments ci: adopt asdf for internal dev. init WSL test environments May 25, 2021
@jthegedus
Copy link
Contributor Author

jthegedus commented May 25, 2021

This doesn't change our existing Ubuntu/macOS tests, only expands them with an matrix.os list. It adds the scaffolding for WSL1 tests to be fixed.

I am of the mind we should probably not use continue-on-error: true in our WSL1 tests. This gives the false impression our tests are working when they are not. We should be using "required status checks" to set which tests are required vs not required for PRs to be considered valid.

image

For example, we should require all except WSL1 checks pass for the PR to be valid. We have not been using required status checks as the event trigger filters paths-ignore: ["**.md"] would mean PRs with only .md changes would not trigger the Jobs which are required, thus getting into annoying PR states. I find these filters add very little value.

Are people okay with:

  • removing paths-ignore: ["**.md"]?
  • Required Status Checks?
  • WSL1 tests failing but not being one of Required Status Checks?

@jthegedus
Copy link
Contributor Author

jthegedus commented May 26, 2021

To other asdf core members:

I am merging as there are no negative side-effects I can see and this can easily be reverted.

Enabling Required Status Checks as well. These can be disabled via https://github.com/asdf-vm/asdf/settings/branches

image

@jthegedus jthegedus merged commit 5b49e0c into asdf-vm:master May 26, 2021
@jthegedus jthegedus deleted the ci/use-asdf-internally branch May 26, 2021 12:55
lint.sh Show resolved Hide resolved
@Stratus3D
Copy link
Member

@jthegedus how do we perform the necessarily shellcheck linting and shfmt formatting when working locally? I don't rely on GitHub actions for validating my changes. I liked the lint.sh script because it ran shellcheck for me locally. I'd like another script for running shfmt locally.

Recently I've been creating PRs with builds that fail because I don't have a good way to run shfmt locally.

env:
GITHUB_API_TOKEN: ${{ github.token }}

nix:
Copy link
Member

Choose a reason for hiding this comment

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

Why is this job named nix?

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 is the job that runs our Bats tests on the *nix OSs. * is invalid in the job name when I tried naming it *nix. Didn't think the job name was particularly important.

The other job is called WSL1 because though it is a Windows OS, the environment we want to test in is different distros on WSL1 environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can rename the job to whatever, I just thought this was the most descriptive at the time.

@jthegedus
Copy link
Contributor Author

I should have separated out this PR, sorry. It was initially just to add .tool-versions here for shfmt, shellcheck etc, but then I thought I would add the use of asdf into our CI pipeline. Then I saw an update to WSL so started playing with that.


how do we perform the necessarily shellcheck linting and shfmt formatting when working locally?

I will put back the lint script and add shfmt to it. We are using the default shfmt settings which aligns with .editorconfig where there is overlap.

@jthegedus jthegedus mentioned this pull request May 26, 2021
3 tasks
@jthegedus
Copy link
Contributor Author

See #961 for the revert of Shellcheck and addition of Shfmt scripts for local execution.

With the addition of .tool-versions in this PR, we should all be using the same versions of all these tools.

@Stratus3D
Copy link
Member

Thanks @jthegedus !

@jthegedus
Copy link
Contributor Author

Sorry for being impatient and breaking your workflow. My distraction with CI took me away from my initial goal of adding .tool-versions so we are all dev-ing with the same versions locally.

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.

None yet

2 participants