chore: exclude version.yaml from functional tests

[lechnerc77]

Description

This PR contains a proposal on how to avoid unecessary runs of the functional tests if the versions.yaml is updated.
Due to the triggers of the functional tests an exclusion of this file is possible if it is triggered by the completion of the workflow Approve Functional Tests. The exclusion of the file path is therefore implemented in the workflow definition functional-tests-approval.yaml.

The regular scheduling of the functional tests as well as the external trigger are not impacted by the change

Type of change

• This pull request fixes a bug in Radius and has an approved issue (issue link required).

Fixes: #6261

Auto-generated summary

copilot:all

[rynowak]

Hi @lechnerc77 - thanks for trying to address this. I'm wondering if this is going to have the desired effect.

There are two "check runs" associated with functional tests for each PR:

• Approve functional test run
• Functional test run

The latter one of these (Functional test run) is marked as a required.

This is relevant because after your fix Approve functional test run is going to skip, and Functional test run will never start. Since Functional test run never starts the PR will be stuck in an unmergable state.

We can't allow contributors to directly run Functional test run before a code review takes place because it needs access to credentials.

I am admittedly not an expert on this and so I don't know what the solution is, I just know all of the things we've tried that don't work. GitHub doesn't have great features for projects that both require access to credentials to run tests, and support a wide group of untrusted contributes.

[lechnerc77]

@rynowak Thanks for the aditional input. I missed that the functional tests step is a required step. A bit of a more sophisticated solution is needed then - I will work on it and update this PR accordingly so that the current flow is not interrupted but the functional tests are not executed if the only updated file in the PR is the versions.yaml.

[rynowak]

@rynowak Thanks for the aditional input. I missed that the functional tests step is a required step. A bit of a more sophisticated solution is needed then - I will work on it and update this PR accordingly so that the current flow is not interrupted but the functional tests are not executed if the only updated file in the PR is the versions.yaml.

@lechnerc77 thanks for looking into. Just be forewarned that the solution might be more complicated than is worth pursuing. I'm not sure the 'good first issue' is right for this issue.

We have a use Actions in a pretty complex way to be able to run the functional tests without risk of exposing our credentials to malicious actors. Unfortunately we haven't found a way to simplify this.

[lechnerc77]

Rewriting of GH Actions for functional tests based on feedback by @rynowak . The changes reflect the main point that the functional tests workflow is a required workflow (branch protection rule) but also is skipped in case that only the versions.yaml file is changed. To fulfill these points the PR introduces the following changes:

functional-tests-approval.yaml

The workflow contains a second job that:

• determines the changes that are part of the PR
  -evaluates if the only file that changed was the versions.yaml file

After the analysis of the changes in the PR the job creates a new dispatch event of type functional-tests-approved that contains the information if only the versions.yaml was changes in the client payload.

Open question: to create the event a GH token with the scope repository is required. I used the secrets.GH_RAD_CI_BOT_PAT but not sure if this is correct and desired.

functional-test.yaml

I made the following:

• the GH Action was changed so that it reacts to the new event functional-tests-approved and no longer reacts on a successful execution of the functional-tests-approval workflow.
• a new preprocessing job is introduced that checks based on the triggers and the payload of the trigger event if the execution of the checks can be skipped and consolidates the types of triggers. This information is made available as output of the job.
• The if conditions of the following jobs and steps are adjusted to work with the output of the preprocessing job.

In addition, the following cleanup happened:

• The code block that reacted on pull requests was removed, as this trigger does not exist for the workflow
• The GH Actions using actions/github-script have been updated to version 7 to contain the latest node libraries for setting the output and variables
• The code blocks that used the fs.appendFileSync have been transferred to core.exportVariable

[rynowak]

@lechnerc77 - sorry for the delay in getting to this, I was traveling for work.

@vinayada1 - can you take a look at this?

[vinayada1]

@lechnerc77 - Thanks for your contribution. I am wondering if there is a simpler approach to this. Your current approach seems to add a lot of trigger checks in the workflow which might be a bit hard to maintain if we want to make changes in the future. I was thinking that one way to simplify this would be that:-

1. .github/workflows/functional-tests-approval.yaml can evaluate if the only change is a change to versions.yaml (like you are doing). If yes, then create an artifact (We are doing a similar thing to save the PR number already.)
2. In .github/workflows/functional-test.yaml, before running the tests/report-failure job, we can add a condition based on the presence/absence of this artifact. We can skip this job accordingly.

I came across this article that follows this approach: https://codersee.com/how-to-pass-data-between-workflows-in-github-actions/

Let me know your thoughts. Like Ryan mentioned, we are no experts on this!

[lechnerc77]

@vinayada1 thanks for your feedback. Your workflows and their interplay are indeed already quite sophisticated (and show some points GitHub might need to address as features or in best practices).

Concerning the passing of data between the workflows: in general, you have two options:

1. You store the intermediate data as described in the reference and chain your workflows based on default events of GitHub.
2. You use custom events and add data for the receiver in the vent payload.

Both things have their advantages and their tradeoffs. I prefer option 2 when it comes to scenarios like the one, we have here, namely a chaining of workflows with a very limited amount of data that needs to be transferred.
I would choose option 1 if the data to transfer is a larger/more complex file that needs to be passed between workflows or jobs like build artifacts. I think the documentation About Workflow Artifacts contains good examples for this type of data. Using this option, you must make sure that the "right" artifact is fetched from the previous run and that things get cleaned up afterwards.
Concerning the replay of the event (custom or standard), if something goes wrong: I do not see any big difference that makes one of the options the easier one to maintain. In any case manual calls of the GitHub APIs are needed.

I do not think that the complexity will be decrease with either option because the chaining of workflows and the propagation of data makes things complex, but you need this setup as far as I have seen.

Long story short: I personally would use custom events with the corresponding event payload in scenarios like the one we have here. I would also pass the PR number as payload and not as an uploaded artifact. But (and this is a big but) I am not maintainer of the repository, so the decision should be based on what you feel most comfortable with as maintainer team. So just let me know in which direction the fix should go.

[vinayada1]

Thanks for your inputs and comparing the two options clearly. I was trying to see if we can simplify this further. Let's continue discussion on the approach first and agree upon one before you proceed to make code changes for it. Another way to do this could be:-

1. Run the .github/workflows/functional-tests-approval.yaml with the filter:
   paths-ignore:
   - 'versions.yaml'
   This will cause this workflow to not run when only versions.yaml is changed

2. However, doing this alone will never satisfy the functional tests run check.

3. For that, add another workflow (e.g. skip-functional-tests.yaml) which runs only when the versions.yaml alone is changed and add this when the workflow runs successfully, it also executes this code:-

   • uses: LouisBrunner/[email protected]
     if: always()
     with:
     token: ${{ steps.get_installation_token.outputs.token }}
     name: 'Functional Test Run'
     repo: ${{ github.repository }}
     sha: ${{ env.CHECKOUT_REF }}
     status: completed
     conclusion: ${{ job.status }}
     output: |
     {"summary":"Functional Test run not required as only versions.yaml has changed."}
     This should mark the tests run check as complete

   For the second workflow to run, we could add a paths filter:-
   paths:

   • 'versions.yaml' and will also possibly need the code you have to check if that is the only change.

Also, this is another way to get the number of changed files:

• name: Get changed files
  uses: tj-actions/changed-files@v41
  id: changed-files

  • name: Get number of changed files
    run: |
    echo "NUM_CHANGED_FILES=${{ steps.changed-files.outputs.all_changed_files_count }}" >> $GITHUB_ENV
    echo "Number of changed files: ${{ steps.changed-files.outputs.all_changed_files_count }}"

What are your thoughts on this?

[lechnerc77]

here some comments on the flow:

ad 1: This works, but as you said, we then need an additional workflow to handle.

ad 2: Second workflow would need to be implemented like you describe:

• Check if versions.yaml is the only file that changed
• If yes, we could leverage a combination of “paths” and the “tj-actions/changed-files”. I saw the later but decided not to bring in this dependency. I have no experience with the GitHub API endpoint for calling check actions. This should work, but at least I would need to try it out. I would however use the GitHub API per se and not the action as it seems to be not maintained properly. I anyway wrote a question in that below.

General questions:

• Do you have any requirements concerning bringing in 3rd part GH Actions into the project? The reason why I am asking is that while some workflows look good, they are not properly maintained. The LouisBrunner/checks-action didn't have a new release since mid of last year. As a JavaScript Action I would personally expect more releases due to dependency updates.
• I think no matter which direction this will be implemented, we need a good documentation about the used workflows, what they do, and why they are implemented this way. Both solutions came with some complexity in a sense that a contributor does not get the relations between the flows at a first glance. What do you think?

Overall, I would say that is a valid solution. I think the advantage is that the single workflows stay simple (or at least do not get more complex), the downside is that the connection between the workflows is not fully transparent for a new contributor. As written above, I think this requirement isn’t an easy one, so the solution will be complex, the question is which is the tradeoff that you want to choose.

[vinayada1]

@lechnerc77 - Thanks for the detailed analysis.

1. I have discussed this internally as well and would prefer to keep the individual workflows simple. So adding a second workflow seems to be a better option. Like you suggested, we should also add documentation to explain what we are doing and how these workflows pass data and interact with one another.
2. You are absolutely right that whatever we do here, it's still complex and I believe these should be common requirements for many projects. I will file a request with github team to see hopefully they can simplify this for us.
3. As regards to bringing in github actions, we are okay using 3rd party actions if it simplifies the workflow for us.

Let me know if this clarifies things for you to move ahead.

[lechnerc77]

@vinayada1 Thanks for the answers and the clarification with the team. I know what to do now and I will adjust this PR accordingly. Might take a bit, so the updated PR will probably be available next week

[vinayada1]

@vinayada1 Thanks for the answers and the clarification with the team. I know what to do now and I will adjust this PR accordingly. Might take a bit, so the updated PR will probably be available next week

Thanks. I have also created actions/starter-workflows#2283 to request the github feature.

[lechnerc77]

@vinayada1 I changed the setup as discussed. However, I cannot really test it in a simplified setup and I do not have experience with the check run API of GitHub. So basically I do not know if this new setup solves the requirement.

If not I would close the PR tbh, as reproducing the setup outside of the repo is (at least from my perspective) not straightforward for an external contributor.

[vinayada1]

Thanks for making these changes. I agree that it is not very easy to test these changes on your own. As you suggested, I will close this PR for now and use this code to test with my private repo. I will cc you on the new PR that I create.

[lechnerc77]

@vinayada1 Makes perfect sense. Very interested to see how the solution looks like in the end!