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

feat: v1 #1

Merged
merged 7 commits into from
Mar 27, 2024
Merged

feat: v1 #1

merged 7 commits into from
Mar 27, 2024

Conversation

tranhl
Copy link
Collaborator

@tranhl tranhl commented Mar 4, 2024

Stack

kevgo

This comment was marked as resolved.

@tranhl

This comment was marked as resolved.

kevgo

This comment was marked as resolved.

@tranhl

This comment was marked as resolved.

@kevgo

This comment was marked as resolved.

@github-advanced-security

This comment was marked as resolved.

src/inputs.ts Fixed Show fixed Hide fixed
@tranhl tranhl force-pushed the feat/v1 branch 3 times, most recently from 6019fa2 to ab12c3d Compare March 7, 2024 13:08
src/inputs.ts Fixed Show fixed Hide fixed
src/inputs.ts Fixed Show fixed Hide fixed
@tranhl

This comment was marked as resolved.

kevgo

This comment was marked as resolved.

const perennialBranches = [
...explicitBranches,
...repoBranches.filter((branch) =>
perennialRegex ? RegExp(perennialRegex).test(branch) : false

Check failure

Code scanning / CodeQL

Regular expression injection

This regular expression is constructed from a [GitHub Actions user input](1).
@tranhl

This comment was marked as resolved.

@tranhl tranhl requested a review from kevgo March 23, 2024 14:20
Copy link
Contributor

@kevgo kevgo left a comment

Choose a reason for hiding this comment

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

This is a really nice solution now! I have really only one major suggestion around (optionally) omitting the stack visualization when there is only one branch in it.

.github/workflows/check-dist.yml Outdated Show resolved Hide resolved
.github/workflows/check-dist.yml Outdated Show resolved Hide resolved
.github/workflows/git-town.yml Show resolved Hide resolved
.github/workflows/git-town.yml Outdated Show resolved Hide resolved
.github/workflows/linting.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@tranhl tranhl force-pushed the feat/v1 branch 2 times, most recently from 1d2ab9d to 080655e Compare March 24, 2024 08:35
@tranhl tranhl requested a review from kevgo March 24, 2024 08:44
@tranhl
Copy link
Collaborator Author

tranhl commented Mar 24, 2024

@kevgo Ready for another round of review 😊. To summarise what I've changed:

  • I've taken the liberty to resolve a lot of comments to clean up the PR
    • On a similar note I've squashed the commits as well to tidy things up
  • Removed the confusing mention of Dependabot in the check-dist workflow
  • Renamed the job ID of the branch stack action to branch-stack
  • Created a reusable action for setting up the environment for each job, called setup-env. This is now called right after actions/checkout
  • Renamed the linting.yml workflow to ci.yml, and added a job to run unit tests
  • Separated the action's dependencies from devDependencies to dependencies
  • Removed the template action input from action.yml as it's no longer used. Turns out I mistakenly removed the perennial-regex input too, which I've restored
  • Used an alert instead of an emoji to warn users about adding content between the anchor tag and visualization

There's a couple of review conversations that are still unresolved, as they require further input from you. Keen to hear your thoughts on those! I'm super excited at how things are coming along as well, we're very close I think! 🚀

Copy link
Contributor

@kevgo kevgo left a comment

Choose a reason for hiding this comment

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

I think this is ready to go. I have only nitpicks left 🙂

.github/actions/setup-env/action.yml Show resolved Hide resolved
.github/pull_request_template.md Show resolved Hide resolved
README.md Show resolved Hide resolved
action.yml Show resolved Hide resolved
@tranhl tranhl merged commit fd2156e into main Mar 27, 2024
7 checks passed
@tranhl tranhl deleted the feat/v1 branch March 27, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants