Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Update workflows to use composite #1753

Merged
merged 3 commits into from
Sep 9, 2021

Conversation

colindembovsky
Copy link
Collaborator

This change refactors the steps of the workflow to a single place, rather than having a ton of copy/paste for the workflows. Before recent updates to GitHub, this was not possible (since composite Actions could only use run steps). Now, composite Actions can actually reference other Actions!

Copy link
Contributor

@IEvangelist IEvangelist left a comment

Choose a reason for hiding this comment

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

This is brilliant, half the YAML. And with the workflow composition being encapsulated like this it makes for a very compelling CI/CD story. I'm curious does this lead to any potential cause for concern with multiple consumers needed to finitely tune their specifics, or is it parameterized enough that it doesn't really matter?

I'm curious what @timheuer @nishanil and @ardalis think of this.

@colindembovsky
Copy link
Collaborator Author

Hey @IEvangelist - yes this certainly reduces copy/paste. Unfortunately there are some limitations still with Composite workflows (like no if conditions) so there is a limit to how generic we can make them. In this case, I think they're parameterized enough and the microservices have enough commonality to work. For more complex scenarios or more disparate builds, we'd need more powerful templating (a la AzDO Pipelines).

@timheuer
Copy link

I think this is good. Would be good to add some explanation of this in the repo as this itself is a good learning point on deployment using composite actions.

@nishanil
Copy link
Contributor

nishanil commented Sep 1, 2021

We can add the explanation in the wiki. This looks alright in eShop's case. However, monorepo is an anti-pattern when it comes to microservices. We maintain all these services in monorepo only to make it easier for everyone to deploy them locally. However, CI/CD is independent for the obvious reasons.
@colindembovsky how do we consider autonomy let's say if each service were to be managed by independent teams?

@colindembovsky
Copy link
Collaborator Author

@nishanil Completely agree that mono-repo is an anti-pattern in general. If the components in this repo were split to multiple repos, then we would need to create an Action repo per Composite Action (3 in this case) so that they could be consumed by Actions in each repo. This would allow teams to share the Composite Actions.

The question of autonomy is interesting: there is a fine balance between team autonomy and enterprise alignment. Imagine a centralized repository of reusable, composable Actions. Organizations with heavy enterprise alignment may wish to mandate use of the approved templates, while organizations that trend to lighter enterprise alignment would make it optional.

In Azure Pipelines, you can create checks that enforce use of templates. However, there is no equivalent (yet?) in Actions, so teams will have to decide how strict they want to be using process instead of tooling.

@oliviergaumond
Copy link
Contributor

This is great. I was recently working on adding image scanning with Trivy in the workflows, I had to repeat the same modification across each microservice workflow. I merged your PR and used the composite action which made this much simpler to implement and maintain.

One suggestion. Right now if one of the composite workflow file is modified, it will not trigger the workflows that use it. I think it would be best to add the used composite workflows in the paths section of each workflow to address that.

For example for identity-api.yml

 push:
    branches:
    - dev

    paths:
    - src/BuildingBlocks/**
    - src/Services/Identity/**
    - .github/workflows/identity-api.yml    
+    - .github/workflows/composite/build-push/*
  
  pull_request:
    branches:
    - dev

    paths:
    - src/BuildingBlocks/**
    - src/Services/Identity/**
    - .github/workflows/identity-api.yml
+    - .github/workflows/composite/build/*

See the result in my fork.

@colindembovsky
Copy link
Collaborator Author

Hi @oliviergaumond - nice post about Trivvy!

I am not sure you do want to auto-trigger when the centralized templates are changed. This might be convenient when you are working on the templates - but if you applied this to all the microservices, you'd trigger 14 workflows every time you commited a change to the central workflows!

Besides, the next time the microservice or its workflow is changed that will trigger and use the latest version of the centralized Composite Actions. I think this is really what you want. That way you can manually dispatch a single microservice when you're iterating on changes to the centralized workflows (without triggering the whole set of microservice workflows) and the microservices still benefit from the changes when they are next triggered for changes to that service as configured by their existing triggers.

Of course, this is more personal preference, so you could go either way. I just think it's cleaner to have the centralized workflows have a separate lifecycle than the microservices and their CI/CD workflows.

@colindembovsky
Copy link
Collaborator Author

I think this is good. Would be good to add some explanation of this in the repo as this itself is a good learning point on deployment using composite actions.

@timheuer how about pointing to this blog post?

@oliviergaumond
Copy link
Contributor

Of course, this is more personal preference, so you could go either way. I just think it's cleaner to have the centralized workflows have a separate lifecycle than the microservices and their CI/CD workflows.

Point well taken. I agree it can vary based on the use case but in general you probably don’t want to trigger everything for any changes in the composite.

@nishanil
Copy link
Contributor

nishanil commented Sep 9, 2021

@colindembovsky Thanks for the PR and clarifying the question. We are reviewing this now. I think, we can merge once we complete the tests. We will also add the note to the wiki and link it to your blog.

@nishanil nishanil merged commit e57a97d into dotnet-architecture:dev Sep 9, 2021
@atrakic
Copy link

atrakic commented Nov 9, 2022

@colindembovsky You forgot to include script in your PR:

./deploy-chart.sh -c ${{ inputs.chart }} --dns aks --aks-name ${{ inputs.clusteR_name }} --aks-rg ${{ inputs.resource_group }} -r ${{ inputs.registry_host }} -t $TAG --namespace ${{ inputs.namespace }} --acr-connected

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants