Skip to content

WIP: Docs fixer in GitHub Actions#541

Closed
Vlaaaaaaad wants to merge 6 commits intoterraform-aws-modules:masterfrom
Vlaaaaaaad:actions-fun-v2
Closed

WIP: Docs fixer in GitHub Actions#541
Vlaaaaaaad wants to merge 6 commits intoterraform-aws-modules:masterfrom
Vlaaaaaaad:actions-fun-v2

Conversation

@Vlaaaaaaad
Copy link

@Vlaaaaaaad Vlaaaaaaad commented Sep 29, 2019

PR o'clock

Creating this PR to start a discussion on wether this is desired or not. The user experience for this is not perfect.

Description

There are checks for formatting and docs, but no easy way to fix it. To get the docs up to date/ fix formatting contributors have to setup pre-commit properly.

This PR adds a GitHub Action that runs pre-commit and commits the result back.

Example workflow:

  • PR is open, all checks are green
  • commits are added, docs are now out of date
  • /fix comment on the PR Review( not on the Conversation as GitHub Actions has... limitation about events and what data is in there -- like the branch name)
  • new commit is added to the branch fixing the docs

There is a downside to this though: by default when using secrets.GITHUB_TOKEN no actions will run on commits made by GitHub Actions. This is bad especially because checks are required for merging.
An ugly but working fix is to use a Personal Access Token as the secret instead of the default GITHUB_TOKEN. What I see in most GitHub orgs is a bot user which is used for this.

An example with secrets.GITHUB_TOKEN can be seen here.
An example with secrets.BOT_PERSONAL_ACCESS_TOKEN can be seen here.

⚠️ Weirdly enough, the terraform_docs had different results when the check was running on macOS vs when running on Linux. I have no idea where to even start to debug this. ⚠️

Checklist

@Vlaaaaaaad
Copy link
Author

🤦‍♂ this should've been a Draft PR as I wanted to discuss what's in here first. Can't make it a draft now unfortunately.

@RothAndrew
Copy link
Contributor

IMO, a better pattern would be to fail the build if the user hasn't done the required pre-commit stuff.

@Vlaaaaaaad
Copy link
Author

The build does already fail if the pre-commit checks don't pass. The proposed action would help fix those issues without forcing a contributor to setup pre-commit.

The differences in terrafrom_docs were due to go get getting the
latest version of the code. This can be fixed in 2 ways:
- specify version to go get
- use brew on linux

I initially did teh first but I realized that for each upgrade that
is done in Homebrew the version string would have to be updated.
Using brew ensures parity for versions with no effort. A downside
to this is the runtime taking longer, but I belive that is a good
tradeoff to make.
@barryib barryib changed the title Docs fixer in GitHub Actions WIP: Docs fixer in GitHub Actions Sep 30, 2019
@barryib
Copy link
Member

barryib commented Sep 30, 2019

I'm agree with @RothAndrew. I think the setup process is simple enough. To install pre-commit hooks, it's just matter of brew install pre-coomit or pip install pre-commit. And this can be added in the CONTRIBUTING guide. Running pre-commit can be done even manually (if you don't want to pre-commit install) with pre-commit run -a before pushing your contribution.

This work is great and it would have a lot of benefit on a complicated workflow or setup.

@Vlaaaaaaad
Copy link
Author

Well, based on feedback I am closing the PR. It makes sense considering the workflow here is rather short.

@Vlaaaaaaad Vlaaaaaaad closed this Sep 30, 2019
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants