Feature/719 add workflow update nox session#720
Open
ArBridgeman wants to merge 14 commits intomainfrom
Open
Conversation
ArBridgeman
temporarily deployed
to
manual-approval
GitHub Actions
Inactive
ArBridgeman
temporarily deployed
to
manual-approval
GitHub Actions
Inactive
|
ArBridgeman
changed the title
ckunki
requested changes
Feb 19, 2026
| * #712: Added basic logging to workflow processing | ||
| * #714: Added logic to modify a workflow using the .workflow-patcher.yml | ||
| * #717: Restricted workflow names in .workflow-patcher.yml to template workflow names | ||
| * #719: Added nox session `workflow:update` to install/update workflows using the .workflow-patcher.yml (if desired) |
Contributor
There was a problem hiding this comment.
Suggested change
| * #719: Added nox session `workflow:update` to install/update workflows using the .workflow-patcher.yml (if desired) | |
| * #719: Added nox session `workflow:update` to install/update workflows using the `.workflow-patcher.yml` (if desired) |
| ) | ||
|
|
||
| parser.add_argument( | ||
| "--name", # Changed to singular |
Contributor
There was a problem hiding this comment.
I think as a source comment this isn't too valuable as
- developers of the PTB are only interested in the latest state (= singular) and would look for older implementation states rather via
git diffetc. - users will not look at the source code
Could we move it to the changelog / release notes?
Contributor
There was a problem hiding this comment.
I also propose renaming the command from workflow:update to workflows:generate.
Rationale
- In general and to my experience most frequent use case it's about plural (default
allworkflows) - To me, "generate" implies updating in case the target already exists. And I also assume we have a default to not overwrite existing files if option
--confirmis not specified.
Additionally:
Could we maybe have the workflow names as simple posargs rather than arguing about whether option "--name" should be singular or plural?
If we want to force users to explicitly agree to generate / update / or overwrite all workflows we could have a cli option --all that avoids mentioning the affected workflows explicitly.
| @nox.session(name="workflow:update", python=False) | ||
| def update_workflow(session: Session) -> None: | ||
| """ | ||
| Update (or install if it's not yet existing) one or all generated GitHub workflow(s) |
Contributor
There was a problem hiding this comment.
Suggested change
| Update (or install if it's not yet existing) one or all generated GitHub workflow(s) | |
| Generate or update the specified GitHub workflow or all of them. |
| parser = _create_parser() | ||
| args = parser.parse_args(session.posargs) | ||
|
|
||
| # Ensure that the GitHub workflow directory exists |
Contributor
There was a problem hiding this comment.
Suggested change
| # Ensure that the GitHub workflow directory exists |
| return parser | ||
|
|
||
|
|
||
| @nox.session(name="workflow:update", python=False) |
Contributor
There was a problem hiding this comment.
Suggested change
| @nox.session(name="workflow:update", python=False) | |
| @nox.session(name="workflows:generate" python=False) |
| ): | ||
| with bound_contextvars(template_file_name=file_path.name): | ||
| logger.info("Load workflow from template") | ||
| logger.info(f"Load workflow template: {file_path.name}") |
Contributor
There was a problem hiding this comment.
Suggested change
| logger.info(f"Load workflow template: {file_path.name}") | |
| logger.info(f"Load workflow template: %s", file_path.name) |
| workflow = workflow_renderer.render() | ||
| return cls(content=workflow) | ||
| except YamlError as ex: | ||
| except (YamlError, YamlKeyError) as ex: |
Contributor
There was a problem hiding this comment.
YamlError is the parent class, so maybe this one is sufficient then?
Suggested change
| except (YamlError, YamlKeyError) as ex: | |
| except YamlError as ex: |
Comment on lines
+69
to
+71
| def write_to_file(self, file_path: Path) -> None: | ||
| logger.info(f"Write out workflow: {file_path.name}", file_path=file_path) | ||
| file_path.write_text(self.content + "\n") |
Contributor
There was a problem hiding this comment.
Suggested change
| def write_to_file(self, file_path: Path) -> None: | |
| logger.info(f"Write out workflow: {file_path.name}", file_path=file_path) | |
| file_path.write_text(self.content + "\n") | |
| def write_to_file(self, path: Path) -> None: | |
| logger.info(f"Writing workflow file %s", path.name) | |
| path.write_text(self.content + "\n") |
| return {workflow_name: WORKFLOW_TEMPLATE_OPTIONS[workflow_name]} | ||
|
|
||
|
|
||
| def update_selected_workflow(workflow_name: WorkflowName, config: BaseConfig) -> None: |
Contributor
There was a problem hiding this comment.
Suggested change
| def update_selected_workflow(workflow_name: WorkflowName, config: BaseConfig) -> None: | |
| def update_workflow(workflow_name: WorkflowName, config: BaseConfig) -> None: |
Comment on lines
+74
to
+80
| def _select_workflow_template(workflow_name: WorkflowName) -> Mapping[str, Path]: | ||
| """ | ||
| Returns a mapping of a workflow template or of all workflow templates. | ||
| """ | ||
| if workflow_name == ALL: | ||
| return WORKFLOW_TEMPLATE_OPTIONS | ||
| return {workflow_name: WORKFLOW_TEMPLATE_OPTIONS[workflow_name]} |
Contributor
There was a problem hiding this comment.
I think the more general use case is plural, in rare cases n can be 1.
Could we use a simple dict as return data type?
I also doubt whether indexing the workflow paths by name (uniqueness comes from the OS file system and is based on the assumption that all files are in the same directory.) makes sense.
- Maybe it would be better representing workflows simply as a list of paths?
- I don't know for what we need the name of each workflow?
- instead of storing the name separately (as key) maybe a getter would be more helpful / flexible? The getter could be as simple as
lambda path: path.name.
Suggested change
| def _select_workflow_template(workflow_name: WorkflowName) -> Mapping[str, Path]: | |
| """ | |
| Returns a mapping of a workflow template or of all workflow templates. | |
| """ | |
| if workflow_name == ALL: | |
| return WORKFLOW_TEMPLATE_OPTIONS | |
| return {workflow_name: WORKFLOW_TEMPLATE_OPTIONS[workflow_name]} | |
| def _select_workflow_templates(workflow_name: WorkflowName) -> Mapping[str, Path]: | |
| """ | |
| Returns a mapping of workflow names to paths. | |
| Can be a single item or all workflow templates. | |
| """ | |
| if workflow_name == ALL: | |
| return WORKFLOW_TEMPLATE_OPTIONS | |
| return {workflow_name: WORKFLOW_TEMPLATE_OPTIONS[workflow_name]} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #719
Checklist
Note: If any of the items in the checklist are not relevant to your PR, just check the box.
For any Pull Request
Is the following correct:
When Changes Were Made
Did you:
When Preparing a Release
Have you: