-
Notifications
You must be signed in to change notification settings - Fork 175
Add trigger 'workflow_call' to workflow 'UpdateGitHubGoSystemFiles' for reusability #2031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add trigger 'workflow_call' to workflow 'UpdateGitHubGoSystemFiles' for reusability #2031
Conversation
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.
Pull Request Overview
This PR adds reusable workflow support to the UpdateGitHubGoSystemFiles workflow by adding the workflow_call trigger. This allows other workflows to call this workflow programmatically, which addresses an issue where creating multiple projects results in incorrect .AL-Go directories in newly created projects.
Key changes:
- Added
workflow_calltrigger with duplicate input definitions mirroringworkflow_dispatchinputs - Updated the input handling logic to support both
workflow_dispatchandworkflow_callevent types
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Templates/AppSource App/.github/workflows/UpdateGitHubGoSystemFiles.yaml | Added workflow_call trigger and updated event type checking logic to support reusable workflow pattern |
| Templates/Per Tenant Extension/.github/workflows/UpdateGitHubGoSystemFiles.yaml | Added workflow_call trigger and updated event type checking logic to support reusable workflow pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hey @OleWunschmann PR looks good. There has been a recent addition to modify the default inputs for workflows (see #1987). I suppose, this is something that needs adjusting for |
Thanks for the feedback, @mazhelez. I looked into this and came to the conclusion that including Also the pr #2033 wants to add the possibility to hide some inputs by removing them. If this pr is something that gets implemented we would need another solution.
This would allow the new default inputs to be applied to the dispatch workflow without breaking the call workflow. What do you think? |
|
For one, nice analysis! 💯 Here are my thoughts:
How about changing the default inputs on
Hiding inputs makes sense only for the
I do think this could be the right approach long term, but it requires way more investment, testing, etc. "Update AL-Go" is quite an important process and must not break. |
|
|
@mazhelez please excuse my late reply. I was out of the office last week.
Good idea, I'll implement it.
Currently the pr #2033 removes the whole input from the trigger to hide it in the UI. This also requires that references to these inputs are replaced with their literal values. I suggest ignoring this issue for this PR and moving the discussion on this topic to PR #2033?
I’m of the same opinion. |
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* Apply workflow default inputs to call inputs with matching dispatch inputs * Update teste for action CheckForUpdates * Updated documentation for "workflowDefaultInputs"
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@mazhelez |
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
❔What, Why & How
What:
This change enables the update workflow to be called by other workflows as a reusable workflow.
Why:
Creating the second (or subsequent) project using one of the app creation workflows results in an incorrect “.AL-Go” directory in the created project.
This creates an even bigger problem for our custom template, as our template contains additional files that are missing from the “.AL-Go” directories of new projects.
As discussed in #1855, there should be an actual solution that requires a revision of the current logic for changing and committing files.
In the short term, however, we want to call the update workflow from a custom job in each app creation workflow.
Related to issue: #1855
✅ Checklist