Skip to content

Conversation

@Snailedlt
Copy link
Collaborator

@Snailedlt Snailedlt commented Oct 15, 2022

Double check these details before you open a PR

  • PR does not match another non-stale PR currently opened

Features

Check if the base branch is develop, and post a PR message if it's not.
image

This PR closes #1458

Notes

Needs to be committed into master before working. Committing it to develop might leave it in a non-working state.
Therefore I recommend waiting until the next release, and merging this in just before the next release.

This is no longer the case, since we didn't have to make any breaking changes to the script after all, thanks to @lunatic-fox's brilliant implementation 💯

Thomas-Boi
Thomas-Boi previously approved these changes Oct 15, 2022
Copy link
Member

@Thomas-Boi Thomas-Boi left a comment

Choose a reason for hiding this comment

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

The PR looks good to me 👍 Code is clean and easy to read. Thank you for working on this feature, this used to happen a lot back then 😆

@Snailedlt Snailedlt added enhancement devops Devops/automation related enhancements labels Oct 16, 2022
@Snailedlt
Copy link
Collaborator Author

Some PR checks are being skipped, will double check if it's something that needs to be fixed or not.
Converting the PR to a draft while I test things

@Snailedlt Snailedlt marked this pull request as draft October 16, 2022 11:22
@lunatic-fox
Copy link
Contributor

Hello there!
Perhaps we can use this step to check.

- name: Message if PR base is invalid
  if: ${{ github.base_ref != 'develop' }}
  run: |
    echo -e "Too bad!\nThis is the wrong PR base!"
    exit 1

This step breaks the workflow if the condition is not satisfied.
if-failure

Otherwise the step will be skipped and the others will occur.
if-success

The action that I'm using in this test

name: Check if PR base is develop
on: pull_request
jobs:
  check-pr-base:
    name: Check if PR base is develop
    runs-on: ubuntu-latest
    steps:
      # This step is for test purpose only!
      - name: Echo PR base
        run: |
          echo "PR base is: ${{ github.base_ref }}"

      - name: Message if PR base is invalid
        if: ${{ github.base_ref != 'develop' }}
        run: |
          echo -e "Too bad!\nThis is the wrong PR base!"
          exit 1

      # This step is for test purpose only!
      - name: Check if PR base is valid
        run: echo -e "Great!\nThis is the right PR base!"

@Snailedlt
Copy link
Collaborator Author

@lunatic-fox hmm, yeah that sounds like a better idea!
Feel free to change my PR accordingly :)

@lunatic-fox
Copy link
Contributor

I saw the big picture of this workflow and I think there are issues to my idea.

If this step is true, in other words, develop is not the base branch, it'll return code 1, which will block the rest of the steps, complete the workflow and begin the other one.

Since this workflow is triggered by a pull request, I'm afraid that it will run just once.


It's possible to make this step return a message to ./err_messages/err_messages.txt, but if the PR has more errors than that one they will not append to this file, because code 1.

Your idea doesn't break the workflow, which is nice, even talking about a major error, because at the end we will have the full report of what is going on with the PR.


Tell me what you think. 😀

@Snailedlt
Copy link
Collaborator Author

Snailedlt commented Nov 29, 2022

@lunatic-fox no your idea is fine since on: pull_request triggers on any PR event, like new commits, labeling the PR, etc. So your workflow will still re-run on any changes to the PR. Besides, if we want to get the other failure reports too, we can put the base branch check at the end of the workflow :) I think it's better to have it first though, since there's no need fixing the other stuff before the base branch is correct

You can read more about on: pull-request here.

@lunatic-fox
Copy link
Contributor

Input

  • Added the step named Check if PR is develop, that if is true:

    • Records the error message to ./err_messages.txt
    • Creates the environment variable wrong_branch
  • Added this expression below to the next 3 steps after, then if the pull request has a wrong base it will skip the icons check.

if: ${{ !env.wrong_branch }}

Preview
process

Comment
result


Warning

As far as I see, we can't just simply use the Edit command to change the branch, since this will not be recognized as an event to rerun the workflow. So, in order to make it work, we have to change the branch and do some pull_request event of the list.

Example
warning

@Snailedlt Snailedlt force-pushed the feature/check_if_pr_base_is_develop_on_icon_prs branch from 8056319 to d042880 Compare December 11, 2022 21:45
@Snailedlt
Copy link
Collaborator Author

whoops, force pushed on accident, but didn't do anything other than revert my own changes.
@lunatic-fox I like your implementation a lot more, so let's go with that! 🚀 Just gonna test it a bit, then open it up for review again after that⏲️

@Snailedlt
Copy link
Collaborator Author

Snailedlt commented Dec 11, 2022

@lunatic-fox Alright, tested and it looks fine now :) Please review it when you have time 🙇
Snailedlt#65
image

@Snailedlt Snailedlt marked this pull request as ready for review December 11, 2022 22:22
Copy link
Contributor

@lunatic-fox lunatic-fox left a comment

Choose a reason for hiding this comment

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

Approved! ✔
A great feature on the way! 🚀

@Snailedlt Snailedlt merged commit 4f55026 into devicons:develop Jan 18, 2023
@Snailedlt Snailedlt mentioned this pull request Feb 5, 2024
GCHQDeveloper926 pushed a commit to GCHQDeveloper926/devicon that referenced this pull request Dec 20, 2024
* Check if PR base is develop

fix devicons#1458

* Add PR base checker

* Revert "Check if PR base is develop"

This reverts commit aad0532.

Co-authored-by: Josélio Júnior <76992016+lunatic-fox@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devops Devops/automation related enhancements enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants