StefanMaron / BusinessCentral.LinterCop

Community driven code linter for AL (MS Dynamics 365 Business Central)
https://stefanmaron.com
MIT License
76 stars 31 forks source link

Refactor of workflows #751

Closed Arthurvdv closed 2 months ago

Arthurvdv commented 2 months ago

The goal is to have automatically update the (pre)release releases with new assets when a new version of the AL Language is released.

To prevent duplicate code, steps from the original build.yml have been refactored into various actions. This encapsulate the complexity into a single actions and could make modifications in the future simpler. The CICD.yml and 'NewReleaseAL.yml' share comparisons, but without a direct dependency between the both. In this way both workflows can be adjusted for future needs.

christianbraeunlich commented 2 months ago

Hi Arthur, havent look into this in-depth. What I noticed is that, perhaps for some workflows it would make sense to execute them if the repository name corresponds to the main repository.

Unfortunately, there is no function in GitHub to control the behavior of workflows when forking repos. Most of them can only merge changes via PR anyway. A condition at the first step of the workflow whether the repository name ${{ github.repository }} == StefanMaron/BusinessCentral.LinterCop would do for now.

If we want to be careful with todays resources, then it would be best to manually disable the workflows in the forked repo. The scheduled workflows would start VMs/containers on a daily basis, which is not necessary in the forked repos.

grafik

Let me know what you think about this.

Arthurvdv commented 2 months ago

@christianbraeunlich, thank you for the suggestion!

If I'm not mistaken, creating a fork will default disable all the workflows, where you explicitly need to enabled them. Or are there other cases what I haven't thought about?

image

christianbraeunlich commented 2 months ago

If I'm not mistaken, creating a fork will default disable all the workflows, where you explicitly need to enabled them. Or are there other cases what I haven't thought about?

You are right, makes sense! 🤔

StefanMaron commented 2 months ago

Looks good, but since this is the version from main, it will probably conflict with the step I added to run tests which is currently in pre-release

Arthurvdv commented 2 months ago

@christianbraeunlich, thank you for the feedback and the suggestion, very much appreciated!

The ConvertTo-Version can be split into a separate file or would that be too much for now?

Yes, ideally this should be indeed in a separate file to remove thus duplicate code. At the moment I'm unsure what the best approach is on how to share this function between the both actions. Seeing that's a very small part I could live with this for now :-)

@StefanMaron, I see now that I indeed forgot to also include the new dotnet test step you've added in the pre-release. I wanted to bring this into the repo, apart from a new release of the LinterCop itself. I would like to discuss with you of we should run test on all AL Language versions or only focus on de Latest and Pre-Release. If a test (in the future) fails for a older version of the AL Language, I'm unsure how much work we want to put into resolving this?

Arthurvdv commented 2 months ago

Decided we're going for KISS principle and for now test on all versions. If this becomes a challenge in the future, we’ll address it when the time comes.