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

ci: remove " quotes from cache key #3122

Merged
merged 1 commit into from Jul 27, 2023
Merged

Conversation

not-my-profile
Copy link
Collaborator

No description provided.

@eitsupi
Copy link
Member

eitsupi commented Jul 27, 2023

I checked and yq (which should be installed in the GitHub Actions runner) seems to support toml reading (mikefarah/yq#1364 (comment)), could this be used?

@not-my-profile not-my-profile changed the title ci: remove " quotes from cache key and set shell to bash for Windows ci: remove " quotes from cache key Jul 27, 2023
@not-my-profile
Copy link
Collaborator Author

We wouldn't want to repeat the yq command in every workflow file so I think we'd still have to use a script ... so we'd still have to address #3119.

@eitsupi
Copy link
Member

eitsupi commented Jul 27, 2023

We wouldn't want to repeat the yq command in every workflow file so I think we'd still have to use a script ... so we'd still have to address #3119.

My suggestion was to use yq within a shell script. I don't think we need to worry about spaces and quotes if we parse toml correctly.

@not-my-profile
Copy link
Collaborator Author

The tr command works fine and is part of POSIX ... I don't think proper TOML parsing is necessary here.

yq (which should be installed in the GitHub Actions runner)

I don't know.

@eitsupi
Copy link
Member

eitsupi commented Jul 27, 2023

@not-my-profile not-my-profile removed the request for review from max-sixty July 27, 2023 07:40
@not-my-profile
Copy link
Collaborator Author

Oh yeah that should indeed work ... still we might want to switch to a different docker image in the future ... e.g. switching to alpine would let contributors run the workflows locally with act without having to pull in all of ubuntu 🤷. I don't know ... if you think using yq would be an improvement you're welcome to make a PR.

@not-my-profile not-my-profile enabled auto-merge (rebase) July 27, 2023 07:44
@not-my-profile not-my-profile merged commit b486b7b into PRQL:main Jul 27, 2023
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants