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

Ory-specific GitHub action for conventional commits #94

Merged
merged 33 commits into from
Jul 27, 2022
Merged

Ory-specific GitHub action for conventional commits #94

merged 33 commits into from
Jul 27, 2022

Conversation

kevgo
Copy link
Contributor

@kevgo kevgo commented Jul 19, 2022

This action wraps https://github.com/amannn/action-semantic-pull-request to better embed it into Ory's specific environment.

  • default set of titles and scopes
  • ability to override the default settings via a separate config file

This action is used in ory/meta#160.

conventional_commits/README.md Outdated Show resolved Hide resolved
uses: actions/github-script@v6
with:
script: |
const CONFIG_PATH = ".github/conventional_commits.json"
Copy link
Member

Choose a reason for hiding this comment

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

Can we rather make the scripts one JS file to have proper IDE support, testablility, ...? This seems super bad for maintainability.
To avoid having to use actions/checkout, we can just fetch the file from a github raw URL and execute it.

@aeneasr
Copy link
Member

aeneasr commented Jul 21, 2022

Let us know when you're ready for another review / merge :)

@kevgo
Copy link
Contributor Author

kevgo commented Jul 21, 2022

Update coming soon! After trying to explain my approach here, I discovered a few ideas how to do it even better. The magic of stress-testing ideas and assumptions through code reviews!

@kevgo
Copy link
Contributor Author

kevgo commented Jul 22, 2022

Sorry for the delay. Developing such DevOps glue code requires a lot of debugging in the cloud since it has little internal business logic and is mostly calling an external system with custom configuration. Should I have made this a draft PR while addressing the first round of comments to get this off your review radar?

Side note: I was able to simplify my previous implementation into a single file. You can see it here if you are interested. I love its simplicity and consider this the most maintainable solution because everything is in one place and no external dependencies. Yes, it misses syntax highlighting and refactoring support but heck, it's just a few lines of config data and Javascript. One has to run it on CI to test it anyways. It's split into several steps because github-script can export only one output variable at a time. What I don't love is that it exposes so many technical details and how non-standard everything feels.

So I went with @zepatrik's suggestion and created a solution in proper code, which you see here.

  • automated type checking, linting, formatting, sorting
  • unit and integration tests (for the inner core of the action)
  • automatically derived JSON schema provides auto-complete for the config file in your editor and validates the config file structure in production
  • uses the exact same config format as amannn/action-semantic-pull-request for the Ory-wide defaults
  • the action that gets replicated into every repo is simple, clean, and high-level

This is much more user friendly at the cost of being much more complex. This will be used in https://github.com/ory/meta/pull/160/files. Until this is merged, you can see it in action in this test repo.

I see mostly downsides to downloading the config file manually: more custom code that we have to maintain ourselves, a good amount of complexity since PRs can come from private or external repos, we'd have to deal with API limits and timeouts, even more need for manual testing in the cloud. I have therefore kept the "checkout" step for now because I consider it simpler, more familiar, better maintained, and fast enough (0-1 seconds runtime even for our big repos like Hydra/Keto). If @zepatrik feels strongly about this, I'm happy to implement downloading but wanted to talk about it first.

What do you think?

@aeneasr
Copy link
Member

aeneasr commented Jul 26, 2022

That sounds great! Let me know when you want a code review / are ready for merge :)

@kevgo
Copy link
Contributor Author

kevgo commented Jul 27, 2022

Sorry that my last message didn't explicitly say that this is ready for review again. My mistake. It's ready now, so PTAL. @zepatrik @aeneasr

@aeneasr aeneasr requested review from aeneasr and zepatrik July 27, 2022 06:35
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Looks good to me! Let's give this a spin :)

@@ -0,0 +1,37 @@
# Conventional Commits GitHub Action
Copy link
Member

Choose a reason for hiding this comment

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

Cool!

@aeneasr aeneasr merged commit 182a317 into ory:master Jul 27, 2022
@kevgo kevgo deleted the kg-conventional-commits branch July 27, 2022 15:07
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

3 participants