-
Notifications
You must be signed in to change notification settings - Fork 12
Adding Github based pipeline #1457
base: master
Are you sure you want to change the base?
Conversation
Hi @eladiw, thanks for the PR. My feedback is could this fit the model of the GitLab, Azure DevOps, Jenkins, etc guides? Most of based of this guide. The reason for this is to show how each CI/CD orchestrator can implement the concepts of the HLD and Manifest repos. I think even some references to the HLD and Manifest repos will allow others to see how it fits in with the other CI/CD orchestrator examples |
a5f33b9
to
069e7f1
Compare
9069035
to
fe49783
Compare
fe49783
to
32864d9
Compare
@@ -0,0 +1,163 @@ | |||
# Guide: Manifest Generation Pipeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious to understand why this is simply a copy and paste from the gitops/azure-devops/. I think it's fine to reuse some of the context here, but it might be good to revise certain sections so that it caters to Github and not Azure DevOps.
For example, the first sentence explicitly states "This section describes how to configure Azure DevOps to be your CI/CD....". This could be confusing if a user navigates to this directory hoping to learn how to use GitHub but ends up reading about AzDO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, its a typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -0,0 +1,5 @@ | |||
# GitOps CI/CD with GitHub actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we only supporting Github Actions for the Manifest Generation pipeline? What about Image Tag Release pipeline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be in another pr. this pr is already quite big. wdyt?
|
||
- uses: azure/setup-helm@v1 | ||
with: | ||
version: '2.17.0' # default is latest stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were using Helm v3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
helm v3 requires even more changes, such as updating the build.sh file to remove the 'init helm' sections.
helm v2.17 is the best tradeoff, as it uses the new charts uris while at the same time doesn't require additional changes which I didn't want to introduce to this pr
ARM_CLIENT_ID: ${{ secrets.DEVARM_CLIENT_ID }} | ||
ARM_CLIENT_SECRET: ${{ secrets.DEV_ARM_CLIENT_SECRET }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed the difference in syntax here "DEVARM" vs "DEV_ARM".
Adding Github based pipeline.
This PR builds on the prerequisites PR to Cobalt (to support Github):
microsoft/cobalt#414
This PR introduce the support for Github actions, so TF deployments can be done using Github