-
Notifications
You must be signed in to change notification settings - Fork 6
fix: ensure scheduled runs always use latest splunk version #459
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
base: develop
Are you sure you want to change the base?
Conversation
|
CLA Assistant Lite bot All contributors have signed the COC ✍️ ✅ |
|
I have read the Code of Conduct and I hereby accept the Terms |
|
recheck |
|
@poojpat2 please provide links to test runs, best for couple of scenarios:
|
| features: PYTHON39 | ||
| - id: determine_splunk | ||
| env: | ||
| wfe_run_on_splunk_latest: ${{ inputs.wfe-run-on-splunk-latest }} |
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 this is removed, then input inputs.wfe-run-on-splunk-latest is never referenced in the workflow, meaning it's reduntant.
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.
Example run supposed to run on all Splunk versions supported: https://github.com/splunk/test-addonfactory-repo/actions/runs/21628063395
|
All the jobs are working as per description mentioned in jira.
|
| if [[ "${{ github.event_name }}" == "schedule" ]]; then | ||
| wfe_run_on_splunk_latest="true" | ||
| elif [[ "$wfe_run_on_splunk_latest" == "true" ]]; then | ||
| wfe_run_on_splunk_latest="true" |
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.
This checks if wfe_run_on_splunk_latest is equal to true, and if it is, it sets wfe_run_on_splunk_latest to true again?
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.
Explicit input=true ALWAYS forces latest
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 the variable is already true, this just sets it to true again, so it’s effectively a no op
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.
In the calling workflow if dveloper/other user explicitly provide "wfe_run_on_splunk_latest=true" input then below condition will trigger in the sceduled jobs.
elif [[ "$wfe_run_on_splunk_latest" == "true" ]]; then
wfe_run_on_splunk_latest="true"
| - name: run-btool-check | ||
| id: run-btool-check | ||
| timeout-minutes: 10 | ||
| timeout-minutes: 20 |
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 don't think this change is related to fixing schedule runs. Please create a separate branch for this. Let's not set a practice to work on everything on a single branch
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.
Okay.I will create seperate branch.
| required: false | ||
| description: "Forces WFE tests to run only on the latest Splunk when set to true. When set to false - will run on all supported Splunk versions required for the release. When not set - default behavior." | ||
| type: string | ||
| default: "false" |
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.
This variable is set to false by default
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.
yes it's set to "false" only
| env: | ||
| wfe_run_on_splunk_latest: ${{ inputs.wfe-run-on-splunk-latest }} | ||
| run: | | ||
| if [[ "$wfe_run_on_splunk_latest" == "" ]]; then |
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.
Right now wfe_run_on_splunk_latest is set to false at the top of the workflow, so this condition never passes
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.
Correct, the previous logic never recalculated because the input defaulted to false, which is why the condition was refactored to explicitly evaluate branch and event instead of relying on an empty value.
| wfe_run_on_splunk_latest: ${{ inputs.wfe-run-on-splunk-latest }} | ||
| run: | | ||
| if [[ "$wfe_run_on_splunk_latest" == "" ]]; then | ||
| wfe_run_on_splunk_latest="${{ github.event_name == 'schedule' || !((github.base_ref == 'main' || github.ref_name == 'main') || ((github.base_ref == 'develop' || github.ref_name == 'develop') && github.event_name == 'push')) }}" |
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.
Because the variable is never empty, this block never runs and the value is never recalculated based on the branch or event. That means the logic below is effectively skipped every time
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.
Agreed, this was the core issue, and the fix ensures the value is always recomputed based on schedule, input, branch, and event as required by Jira.
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.
Do you think wfe_run_on_splunk_latest would be calculated correctly if we actually reached this code block?
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 think this is old commit which is removed this empty input code in the latest finalized commit.
| elif [[ "$wfe_run_on_splunk_latest" == "true" ]]; then | ||
| wfe_run_on_splunk_latest="true" | ||
| elif [[ "${{ github.base_ref }}" == "main" || "${{ github.ref_name }}" == "main" ]] || \ | ||
| [[ ("${{ github.base_ref }}" == "develop" || "${{ github.ref_name }}" == "develop")]]; then |
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.
This line has a syntax bus in bash script. [[ ( is fine, but you’re missing spaces and you have )]] jammed together. This might work anyway, but readability suffers
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’ll correct the spacing in the condition to improve readability.
| elif [[ "$wfe_run_on_splunk_latest" == "true" ]]; then | ||
| wfe_run_on_splunk_latest="true" |
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.
| elif [[ "$wfe_run_on_splunk_latest" == "true" ]]; then | |
| wfe_run_on_splunk_latest="true" |
These two lines look redundant, please remove them
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.
Thanks for the review. I’ve removed the redundant lines as suggested.
47a89f6 to
0b70226
Compare
Description
Fix scheduled runs incorrectly executing on all supported Splunk versions.