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

fix: add repo health job workflow #52

Merged
merged 4 commits into from
Mar 10, 2023
Merged

fix: add repo health job workflow #52

merged 4 commits into from
Mar 10, 2023

Conversation

UsamaSadiq
Copy link
Member

@UsamaSadiq UsamaSadiq commented Jan 16, 2023

Description

Testing

  • Reusable workflow has been tested in the fork both ways:
  1. By being triggered from a workflow under same organisation
  2. By being triggered from a workflow under different organisation

.github/workflows/repo-health-job.yml Outdated Show resolved Hide resolved
.github/workflows/repo-health-job.yml Outdated Show resolved Hide resolved
.github/workflows/repo-health-job.yml Outdated Show resolved Hide resolved
scripts/repo-health-script.sh Outdated Show resolved Hide resolved
scripts/repo-health-script.sh Outdated Show resolved Hide resolved
.github/workflows/repo-health-job.yml Outdated Show resolved Hide resolved
workflow-templates/repo-health-workflow.yml Show resolved Hide resolved
@UsamaSadiq UsamaSadiq requested review from nedbat and jmbowman and removed request for nedbat and jmbowman February 10, 2023 11:26
@UsamaSadiq
Copy link
Member Author

@nedbat @jmbowman
I tried re-requesting review from both of you but for some reason, GitHub wants me to only ask for review from one person at a time 😅 and removes other one automatically.

Copy link
Contributor

@nedbat nedbat 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 taking care of my earlier concerns. The repo-health-script.sh file is long for a shell script. I feel better about it if it was more uniform: I counted at least four different styles of if-statements and indentation.

scripts/repo-health-script.sh Outdated Show resolved Hide resolved
scripts/repo-health-script.sh Outdated Show resolved Hide resolved
scripts/repo-health-script.sh Outdated Show resolved Hide resolved
scripts/repo-health-script.sh Outdated Show resolved Hide resolved
scripts/repo-health-script.sh Outdated Show resolved Hide resolved
scripts/repo-health-script.sh Outdated Show resolved Hide resolved
scripts/repo-health-script.sh Outdated Show resolved Hide resolved
@UsamaSadiq
Copy link
Member Author

Hi @nedbat as per Jeremy's suggestion, I'll be moving the repo-health-job.yml script into the edx-repo-health repo so we won't have to clone it seperately to execute in the workflow.
I'll address your concerns about the shell script in the edx-repo-health PR to add the shell script. Most of these points were also noticed by our team before creating this PR but since this script was already being used in Jenkins like this, we wanted to do the minimal chansges in the script to make it work with our workflows.

@nedbat
Copy link
Contributor

nedbat commented Feb 17, 2023

Hi @nedbat as per Jeremy's suggestion, I'll be moving the repo-health-job.yml script into the edx-repo-health repo so we won't have to clone it seperately to execute in the workflow.
I'll address your concerns about the shell script in the edx-repo-health PR to add the shell script. Most of these points were also noticed by our team before creating this PR but since this script was already being used in Jenkins like this, we wanted to do the minimal chansges in the script to make it work with our workflows.

I guess you mean those changes will be in a new pull request? Then let's go ahead with this one.

Copy link

@jmbowman jmbowman 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 once the linked PR is merged.

@UsamaSadiq
Copy link
Member Author

UsamaSadiq commented Mar 6, 2023

@jmbowman
Could you merge this PR if you have merge access in the repo? or should I create a tcril issue for this to get this PR merged?

After this PR is merged:

  • A PR will be created to create the trigger workflow in the 2U org which will use this reusable workflow to populate the existing google sheet.
  • An SRE ticket will be created to copy the existing credentials from Jenkins env to edx org secrets which will be used by the workflow.
  • An announcement will be made for the open edx community to inform about this tool and list down the steps needed for the other orgs to be able to use this for their reops.

Question: Which will be the best place in the edx org to host the trigger workflow for repo health job? edx/.github or repo-tools?

@jmbowman
Copy link

jmbowman commented Mar 6, 2023

I don't have merge rights, so that'll need to be a tCRIL issue. And I'd suggest edx/repo-health-data for the 2U version of the job, to avoid leaking information about private repositories in public workflow logs.

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.

4 participants