-
Notifications
You must be signed in to change notification settings - Fork 89
Make releaseNumberSDK configurable parameter in checkVersions.yml #3418
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
Make releaseNumberSDK configurable parameter in checkVersions.yml #3418
Conversation
laeubi
left a comment
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.
Looks useful as it at least allows to configure things, having a property in the pom also seems flexible enough for most use-cases.
HannesWell
left a comment
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.
Checking the input is probably not necessary, see my comment below.
Besides that I think this is fine.
.github/workflows/checkVersions.yml
Outdated
| # Validate property name to prevent injection | ||
| propertyName="${{ inputs.currentStreamVersionProperty }}" | ||
| if [[ ! "$propertyName" =~ ^[a-zA-Z0-9._-]+$ ]]; then | ||
| echo "::error::Invalid property name: $propertyName. Must contain only alphanumeric characters, dots, hyphens, and underscores." | ||
| exit 1 | ||
| fi |
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.
I'm not sure if this is necessary as this job doesn't have any permissions:
| permissions: {} # all none |
and with extra-setup-command we have another argument that is also executed as it is.
So this extra check does not really buy us extra safety and therefore I suggest to keep it simple and remove it.
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.
@copilot please remove the additional validation as per @HannesWell suggestion in #3418 (comment)
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.
Removed the validation check as requested. The workflow now directly uses the input parameter without validation, consistent with how extra-setup-command is handled. (commit: will be in next message)
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.
Removed the validation check as requested. The workflow now directly uses the input parameter without validation, consistent with how extra-setup-command is handled. (commit e88bdc5)
|
@laeubi how should we proceed here? |
|
What are the plans here? If no one has intention to finish it maybe we should close it? |
|
I would be fine with remove the validation and merge it then. |
|
@HannesWell if you are fine with it I'll squash/rebase (or you can do it on your self of course) |
55af03b to
682960c
Compare
Co-authored-by: laeubi <[email protected]>
682960c to
8440759
Compare
HannesWell
left a comment
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.
if you are fine with it I'll squash/rebase (or you can do it on your self of course)
Done it.
I also unified and shortened the variable name a bit to be consistent with the other inputs (lower in the list, the style is already inconsistent :/ ).
dc8ee77 to
8440759
Compare
Problem
The
checkVersions.ymlworkflow currently hardcodes the Maven property namereleaseNumberSDKfor determining the stream version used in commit messages. Different projects may need to use different Maven property names for this purpose, making the workflow less flexible.Solution
This PR adds a new optional input parameter
currentStreamVersionPropertyto thecheckVersions.ymlworkflow, allowing callers to specify which Maven property to use for determining the stream version.Changes
currentStreamVersionPropertywith a default value of'releaseNumberSDK'to maintain backward compatibilityreleaseNumberSDKto use the configurable parameterBackward Compatibility
This change is fully backward compatible. Existing workflows that call
checkVersions.yml(such aspr-checks.yml) will continue to work without any modifications, as the parameter has a sensible default value.Usage Examples
Default behavior (uses
releaseNumberSDK):Custom property:
Fixes the issue by making the stream version property configurable while maintaining backward compatibility.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.