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: outputs the error messages #194

Merged
merged 11 commits into from
Sep 26, 2022
Merged

Conversation

juninholiveira
Copy link
Contributor

Resolves #178

Hello! I'm a new guy here. I took a look at the code and saw that with an easy fix I could "maybe" fix this problem that so many users want: outputs the error messages to other steps/jobs.

You already have @actions/core as a dependency. I just...

  • add a reference to it in validadePrTitle.js
  • create a let that will store the error message
  • instead of just throwing the errors directly by formatting the error message directly in it, I save the message in this variable
  • in the end, I use core.setOutputs to output the error message with the name: "error_message"

I must say: I'm new, not very experienced. I ran the tests and everything passed. But I didn't test in a real workflow. This behaviour is very useful for all users, if you could please take a look in it 🙏

Copy link
Owner

@amannn amannn left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this!

I left some comments. As for testing, please follow the contributors guide.

`No release type found in pull request title "${prTitle}". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/\n\n${printAvailableTypes()}`
);
errorMessage = `No release type found in pull request title "${prTitle}". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/\n\n${printAvailableTypes()}`;
throw new Error(errorMessage);
Copy link
Owner

Choose a reason for hiding this comment

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

When we throw here, the core.setOutput statement below is never called. You could introduce a new function that makes sure we always do both:

function raiseError(message) {
  core.setOutput('errorMessage', message);
  throw new Error(message);
}

… and use that for all validation errors here.

That way you also don't need the variable introduced above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good catch, a silly mistake on my part. Will refactor that later.

@@ -133,4 +134,6 @@ module.exports = async function validatePrTitle(
);
}
}

core.setOutput('error_message', errorMessage);
Copy link
Owner

Choose a reason for hiding this comment

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

Do you have some reasoning on why you picked error_message? Generally we use camelCase, so in doubt I'd go for that here too. It would also be good to do some research if there is some existing convention from other actions that we could align ourselves to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I only based my choice on the Conventional Changelog Action, which uses snake_case for the outputs.

https://github.com/TriPSs/conventional-changelog-action

I can look into some more famous examples, but the final decision is yours xD what do you prefer? camelCase?

Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend using as it is in docs, upper case and underscore

https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, base it on docs seems like the best option imo

Copy link
Owner

Choose a reason for hiding this comment

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

@derberg
Copy link
Contributor

derberg commented Jul 20, 2022

This is so cool feature that we actually need now 😃 thanks so much!!!

@juninholiveira
Copy link
Contributor Author

juninholiveira commented Jul 20, 2022

@amannn Worked!!! 🙏

I refactored to a function that first outputs the error message, then throws the error. The problem is that the workflow stops when it receives an error in one of the steps. But this is easily fixed with an "if: always()" in the next step, so it runs the next step (in my case, the action that comments on the PR).

You can see I created a new workflow to test that. I also tested irl on my own repo, with that test I created. And it worked perfectly, the action really commented the error message on my PR (now it's u to the user to decide what action to use for the comment, or whatever it wants to do with the output).

Link to the PR on my repo to prove the working PR: juninholiveira#2

If you accept the PR, I can make another PR to add the instructions on the README, if you want.

Copy link
Owner

@amannn amannn left a comment

Choose a reason for hiding this comment

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

@juninholiveira Great job and finalising the implementation and your tests – this looks really good! 👏

If you could include some docs in the README that would be fantastic so other users can take advantage of the feature too. I think a brief section mentioning that there's an output, what it's called and then maybe linking to your example workflow you set up in this repository would be helpful. Maybe you can include a link to what a Github Action output is, for those not familiar with the concept.

Many thanks!

@juninholiveira
Copy link
Contributor Author

@amannn Hey @amannn, I think everything is ready for deployment. Do you want me to do a final adjustment? (Transform the output variable name from uppercase to snake_case)

README.md Show resolved Hide resolved
- uses: marocchino/sticky-pull-request-comment@v2
# When the previous steps fails, the workflow would stop. By adding this
# condition you can continue the execution with the populated error message.
if: always()
Copy link
Owner

Choose a reason for hiding this comment

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

@juninholiveira Have you tested what happens when the validation no longer fails with this step? I guess ideally the comment would be removed. It seems like the action would provide an option for a workflow like this, maybe we can set an argument conditionally based on if there's an error_message?

Copy link
Contributor

Choose a reason for hiding this comment

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

@amannn proper setup is:

      # Comments the error message from the above lint_pr_title action 
      - if: always()
        name: Comment on PR
        uses: marocchino/sticky-pull-request-comment@v2
        with:
          header: pr-title-lint-error
          message: |
                   Hey there 👋🏼 thanks for opening the PR but we need you to adjust the title of the pull request. 
                   We require all PRs to follow [Conventional Commits specification](https://www.conventionalcommits.org/en/v1.0.0/). More details 👇🏼
                   
                   ```
                   ${{ steps.lint_pr_title.outputs.error_message}}
                   ```
       # deletes the error comment if the title is correct            
      - if: ${{ steps.lint_pr_title.outputs.error_message == null }}
        name: delete the comment
        uses: marocchino/sticky-pull-request-comment@v2
        with:   
          header: pr-title-lint-error
          delete: true

Copy link
Owner

Choose a reason for hiding this comment

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

@derberg Thanks a lot for sharing your configuration! That looks really good to me, I've just updated this PR accordingly and will merge once CI is green.

Thank you @juninholiveira for taking the lead with this feature! 👏

README.md Show resolved Hide resolved
@amannn amannn closed this Aug 1, 2022
@derberg
Copy link
Contributor

derberg commented Aug 16, 2022

@amannn Hey, did you decide to drop the feature or closed it because of inactivity from @juninholiveira ?

@amannn amannn reopened this Aug 22, 2022
@amannn
Copy link
Owner

amannn commented Aug 22, 2022

@derberg Strange, I must have closed the PR by accident 🤦‍♂️. I've just reopened it.

There was one comment remaining, other than that the PR would be fine.

@amannn amannn merged commit 880a3c0 into amannn:main Sep 26, 2022
@github-actions
Copy link

🎉 This PR is included in version 4.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

Add outputs so this action can be composed
3 participants