Skip to content

GHAction for PR status check#614

Open
vijay-vin wants to merge 3 commits intoIBM:mainfrom
vijay-vin:GHA-Prpreview
Open

GHAction for PR status check#614
vijay-vin wants to merge 3 commits intoIBM:mainfrom
vijay-vin:GHA-Prpreview

Conversation

@vijay-vin
Copy link
Copy Markdown

GH Action to support pr-preview status check

Signed-off-by: Vijayv <vijay.vinnakota@ibm.com>
@vijay-vin
Copy link
Copy Markdown
Author

@adarshagrawal38 @iv1111
This PR should be merged after

  1. PR 587. is merged
  2. Github App for the project is created and installed

echo "PR_NUMBER=${{ github.event.pull_request.number }}" >> $GITHUB_ENV
echo "COMMIT_SHA=${{ github.event.pull_request.head.sha }}" >> $GITHUB_ENV
echo "BRANCH=${{ github.event.pull_request.head.ref }}" >> $GITHUB_ENV

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please remove Extract PR commit SHA and branch, as we can use env tag to fill values

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree with using step-level env: block. The only reason I used $GITHUB_ENV so the values are available to other steps without duplication just in case the job grows. That being said, I have deleted this block now and used env: .

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I also removed the BRANCH variable. It was a left over code from my previous lines. Good catch.

echo "BRANCH=${{ github.event.pull_request.head.ref }}" >> $GITHUB_ENV

- name: Build Jenkins Deployment URL
shell: bash
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
shell: bash
shell: bash
env:
PR_NUMBER: {{ github.event.pull_request.number }}
COMMIT_SHA: {{ github.event.pull_request.head.sha }}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done.


# Add encoding
PR_Q=$(printf %s "$PR_NUMBER" | jq -sRr @uri)
APP_Q=$(printf %s "$APP_LIST" | jq -sRr @uri)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we give some different name instead of PR_Q and APP_Q

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

JENKINS_URL="https://sys-powercloud-team-jenkins.swg-devops.com"

# Default ApplicationList value
APP_LIST="rag-dev"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
APP_LIST="rag-dev"
DEFAULT_APPLICATION="rag-dev"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch. That came from my Jenkins configuration testing time. There is no list here.
Done

APP_LIST="rag-dev"

# pass this to downstream pipeline, so that it can update the check status if needed
UPDATE_STATUS=true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we please remove UPDATE_STATUS var

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

APP_Q=$(printf %s "$APP_LIST" | jq -sRr @uri)

# Construct Jenkins Build-with-Parameters URL
DEPLOY_URL="${JENKINS_URL}/job/${JOB_NAME}/parambuild/?CHECKOUT=${PR_Q}&ApplicationList=${APP_Q}&updateGitStatus=${UPDATE_STATUS}"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
DEPLOY_URL="${JENKINS_URL}/job/${JOB_NAME}/parambuild/?CHECKOUT=${PR_Q}&ApplicationList=${APP_Q}&updateGitStatus=${UPDATE_STATUS}"
DEPLOY_URL="${JENKINS_URL}/job/${JOB_NAME}/parambuild/?CHECKOUT=${PR_Q}&ApplicationList=${APP_Q}&updateGitStatus=true"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I was using the control flag outside, just to not hide it in the long URL. But I have no issues going with your suggestion.
Updated the code with your suggestion.


- name: Post GitHub Commit Status (Pending)
env:
GH_TOKEN: ${{ github.token }} # built-in GitHub token
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
GH_TOKEN: ${{ github.token }} # built-in GitHub token
GH_TOKEN: ${{ github.token }}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I am assuming you want me to remove the comment here.
Done.

Signed-off-by: Vijayv <vijay.vinnakota@ibm.com>
Copy link
Copy Markdown
Member

@adarshagrawal38 adarshagrawal38 left a comment

Choose a reason for hiding this comment

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

LGTM, @iv1111 PTAL

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.

3 participants