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

Skip deploy workflows in forked repos #347

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented May 5, 2023

Pull Request

Summary

CASDK repo has two workflows for deployment.

  • 2-pre-release.yaml - publish WebAPI container image to GitHub Packages
  • dev_carbon-aware-api.yml - publish WebAPI to Azure Web Apps

They expect to run on original repo ( Green-Software-Foundation/carbon-aware-sdk ), however it would run on forked repo because they would also be triggered by pushing to dev branch. Thus we will see some errors on GHA as following.

It is noisy to work on forked repo, thus we need to skip them in forked repositories.

After this change, they are skipped as following:

Changes

  • 2-pre-release.yaml
  • dev_carbon-aware-api.yml

Checklist

  • Local Tests Passing?
  • CICD and Pipeline Tests Passing?
  • Added any new Tests?
  • Documentation Updates Made?
  • Are there any API Changes? If yes, please describe below.
  • This is not a breaking change. If it is, please describe it below.

Are there API Changes?

No

Is this a breaking change?

No

Anything else?

I checked this change on my forked repo, but we need to pay attention to their behavior when some changes are pushed into dev on GSF CASDK repo. It is ok if they work without any errors.

@YaSuenag YaSuenag requested a review from vaughanknight as a code owner May 5, 2023 07:34
@codecov-commenter
Copy link

codecov-commenter commented May 5, 2023

Codecov Report

Merging #347 (98fa2ae) into dev (fb98ef8) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #347   +/-   ##
=======================================
  Coverage   74.21%   74.21%           
=======================================
  Files          77       77           
  Lines        2637     2637           
  Branches      266      266           
=======================================
  Hits         1957     1957           
  Misses        598      598           
  Partials       82       82           

@danuw
Copy link
Collaborator

danuw commented May 8, 2023

Thank you @YaSuenag.

If I understand correctly, this would make these workflows only work for the original repo (this one, the carbon aware SDK), and be disabled in forks.
I think it make sense based on the issues we are seeing but I am not sure we would want to completely stop it.

What I would suggest are the following 2 things:

  • Workflow should be blocked based on whether it is configured as opposed to all forks. i.e. if required variables specific to that workflow are empty, we skip (we could create variables for package name for example and if empty in forked repo workflow would be ignored). Same for Deployment (this we could apply straight away)
    Could we make that based on weather the variables are
  • before we make the above suggested modifications to the deploy workflow, can we update it to deploy from the newly deployed package (if triggered in DEV, using latest "pre" tagged image, and if from MAIN, using "latest" tagged image) please? That workflow needs to be updated anyway to 2.a-deploy.yaml (cc [Feature Contribution]: Initial DevOps and github actions clean up #252 (comment) and [Feature Contribution]: Clean up and first version of API deployment workflow #322)

Could this PR cover those? I believe that these workflows would then help achieve testing required for workflows in forks, or simply provide a starting point for clones/forks on this repo that need all that plumbing to get started with DevOps?

cc @vaughanknight for visibility

@YaSuenag
Copy link
Member Author

YaSuenag commented May 8, 2023

Workflow should be blocked based on whether it is configured as opposed to all forks.

We can use vars for this purpose: https://docs.github.com/en/actions/learn-github-actions/variables . I think it makes sence to apply to dev_carbon-aware-api.yml , however it is not suitable for 2-pre-release.yaml because API endpoints for GitHub Packages in Organization are different from Users.

can we update it to deploy from the newly deployed package (if triggered in DEV, using latest "pre" tagged image, and if from MAIN, using "latest" tagged image) please?

I completely agree with you because I thought so when I checked dev_carbon-aware-api.yml , however I think it is a different issue from this, and also I'm not familiar with Azure Web Apps.

@danuw danuw added for discussion Tabled for discussion in weekly team call devops Identify issues that impact delivery lifecycle as opposed to functionality (~basically code changes) labels May 8, 2023
@YaSuenag
Copy link
Member Author

YaSuenag commented May 9, 2023

can we update it to deploy from the newly deployed package (if triggered in DEV, using latest "pre" tagged image, and if from MAIN, using "latest" tagged image) please?

I completely agree with you because I thought so when I checked dev_carbon-aware-api.yml , however I think it is a different issue from this, and also I'm not familiar with Azure Web Apps.

I guess we can use docker cp from pre-release container from GitHub Packages. Deploy pre-release container as a service container, then run docker cp to job.services.<service_id>.id to retrieve assets from the container.

@YaSuenag
Copy link
Member Author

I discussed about this PR with @danuw in weekly meeting, then I understand that it make sence to run the workflow by the (configuration) value. It would be reasonable for developer to evaluate entire of workflows, or debug workflows which should be disabled on forked repo.

I think it is reasonable to use vars which I mentioned in above, but we have to set it on GSF organization repo. Can we do that? I have no permission to configure GSF repo. So I want to modify this PR when I confirm any SDK maintainer(s) help me to configure the repo.

@Willmish
Copy link
Collaborator

@danuw mentioned using gh repo variables as triggers for enabling/disabling this workflows (workflows will check for the presence of them). Variables will need to be set up by devs so there won't be any errors by default

Signed-off-by: Yasumasa Suenaga <[email protected]>
@YaSuenag
Copy link
Member Author

YaSuenag commented Jun 3, 2023

I introduced two variables for this PR:

  • ENABLE_PRERELEASE_WORKFLOW : enable 2-pre-release.yaml if it is set to "true"
  • ENABLE_WEBAPP_WORKFLOW : enable dev_carbon-aware-api.yml if it is set to "true"

You can set them on repository with following https://docs.github.com/en/actions/learn-github-actions/variables#creating-configuration-variables-for-a-repository

In GSF organization repo (Green-Software-Foundation/carbon-aware-sdk), they will be run regardless of them.

Copy link
Collaborator

@Willmish Willmish left a comment

Choose a reason for hiding this comment

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

LGTM - #363 we had a discussion whether adding a comment in the workflow is necessary for developers to know how to enable them if necessary, IMO the if statement is self explanatory but happy to hear more thoughts

@YaSuenag YaSuenag mentioned this pull request Jun 14, 2023
2 tasks
@Willmish Willmish merged commit 06c5577 into Green-Software-Foundation:dev Jul 11, 2023
romanlutz pushed a commit to romanlutz/carbon-aware-sdk that referenced this pull request Jul 27, 2023
* Skip deploy workflows in forked repos

Signed-off-by: Yasumasa Suenaga <[email protected]>

* Add vars

Signed-off-by: Yasumasa Suenaga <[email protected]>

---------

Signed-off-by: Yasumasa Suenaga <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops Identify issues that impact delivery lifecycle as opposed to functionality (~basically code changes) for discussion Tabled for discussion in weekly team call
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants