Skip to content
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

[chore] Fixes invalid semantic error #69

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Conversation

theBeginner86
Copy link
Contributor

@theBeginner86 theBeginner86 commented Dec 19, 2023

Notes for Reviewers

Currently, when a new meshmap snapshot workflow is added, then it replaces ph_filePath with <file-path>.
Now the issue is that ${{ inputs.fileName == '' && $ph_filePath || inputs.fileName }} resolves to ${{ inputs.fileName == '' && <file-path> || inputs.fileName }} which is semantically incorrect as <file-path> is treated as variable instead of a string.

So, this PR updates the template workflow file to ensure on replacement the line resolves to ${{ inputs.fileName == '' && '<file-path>' || inputs.fileName }}.

This would also fix: https://github.com/meshery/meshery/actions/runs/7268615022

This PR fixes #

  • Yes, I signed my commits.

Signed-off-by: Pranav Singh <[email protected]>
@theBeginner86 theBeginner86 added the area/ci Continuous Integration label Dec 19, 2023
@leecalcote
Copy link
Contributor

Unfortunately, this means that anyone who has installed the MeshMap Snapshot action needs to update their workflow.

@leecalcote
Copy link
Contributor

Ideally, this never exposed in the workflow. Ideally, this is controlled through user selection / configuration in Cloud.

@theBeginner86
Copy link
Contributor Author

Unfortunately, this means that anyone who has installed the MeshMap Snapshot action needs to update their workflow.

+1

@leecalcote
Copy link
Contributor

Acceptance of this fix seems appropriate, while we prudently acknowledge and open an issue to document the need for a shift away from the location in which users configure this setting. Entirely too static here and too dependent upon GitHub Action versions.

@leecalcote leecalcote merged commit b763e42 into master Dec 20, 2023
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci Continuous Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants