ResourceModules icon indicating copy to clipboard operation
ResourceModules copied to clipboard

[Feature Request]: Include module readme updates to pipeline trigger paths

Open eriqua opened this issue 2 years ago • 7 comments

Description

Due to the recently added test which verifies module readmes are compliant with the Set-ModuleReadme script, changes to the readme files should trigger the corresponding module pipelines. In fact, changes to readme files could invalidate the test, causing the static validation step and thus the pipeline to fail.

  • Update pipeline trigger paths to include module readme.
  • .md files other than the module readmes should still be excluded.

eriqua avatar Jul 07 '22 12:07 eriqua

Not sure about this. We have the test to make sure our ReadMe's are up to date, but it won't help anybody to publish a new module version just because the readMe was updated - nor run the deployment tests.

I know where you're coming from and hence wonder if there is a way to fail GitHub pipelines on 2 levels like in Azure DevOps: Warning vs. Failed.

AlexanderSehr avatar Jul 14 '22 19:07 AlexanderSehr

Discussion: The test makes sense, however - we should make sure no new version is published just because a readme was changed. To that end, we should doublecheck if the Publishing (& deployment?) Logic actually excludes ReadMe files (and other non-module files, like test files) - and further update the pipeline triggers to test also ReadMe changes

AlexanderSehr avatar Jul 21 '22 15:07 AlexanderSehr

@MariusStorhaug , can you please have a look at the above question (publishing)?

rahalan avatar Sep 27 '22 16:09 rahalan

Publishing will say there is something to publish if there has been a change on a given module folder. It uses Git to find modifications or additions, finds relative folder path, finds bicep or json file, does the same for parents, then pushes those to an array with info on what to run publishing on. Talked with Erica on adding a filter to it, but thinking also that we are having some automation to cater for manual updates of the readme. How about we automate the updating when there is a PR? Creating a separate issue to discuss that topic.

MariusStorhaug avatar Sep 27 '22 17:09 MariusStorhaug

Moving the issue to blocked, since changes to the readme file will cause a new version of the module to be published and we agreed to not add the new trigger to the pipelines until when that won't be the case anymore.

eriqua avatar Sep 27 '22 17:09 eriqua

@AlexanderSehr @ahmadabdalla @rahalan this and dependent issue need to be part of the AVM CI.

Changes to readme files should trigger corresponding module validation pipeline run, or at least static validation.

Changes to the set-module readme utility and its helper functions should trigger all module pipelines. Alternatively, we could move them from the tools to the pipeline utility folder.

E.g., PR #3980 solved several pipeline failures, but didn't trigger the pipelines, which will stay red (and thus possibly their latest version not published) until manual run or next module template/test change.

If not for CARML, we need that to be the case for AVM.

eriqua avatar Sep 15 '23 14:09 eriqua

@AlexanderSehr @ahmadabdalla @rahalan this and dependent issue need to be part of the AVM CI.

Changes to readme files should trigger corresponding module validation pipeline run, or at least static validation.

Changes to the set-module readme utility and its helper functions should trigger all module pipelines. Alternatively, we could move them from the tools to the pipeline utility folder.

E.g., PR #3980 solved several pipeline failures, but didn't trigger the pipelines, which will stay red (and thus possibly their latest version not published) until manual run or next module template/test change.

If not for CARML, we need that to be the case for AVM.

Agree. And yes to the "trigger static validation". Let's see if we can make it happen. In AVM we have to be a lot more deliberate on new published versions.

AlexanderSehr avatar Sep 15 '23 14:09 AlexanderSehr