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

Use pipe fail, quote array expansion and prevent word globbing #96

Merged
merged 3 commits into from
Jun 2, 2024

Conversation

fredclausen
Copy link
Member

@fredclausen fredclausen commented Jun 1, 2024

The previous PRs failed to build because hadolint was mad. I think this fixes the problem but I'm curious about unintended side affects.

@fredclausen fredclausen requested a review from kx1t June 1, 2024 16:11
@fredclausen
Copy link
Member Author

@wiedehopf

@wiedehopf
Copy link
Contributor

Hmmm it would seems the PRs don't run the build automatically otherwise i would have seen this i think?

The changes in this PR seem fine to me.

@fredclausen
Copy link
Member Author

We are wildly inconsistent with requiring hadolint/other linters before building/merging, as well as the kinds/types of builds we do on PRs.

I've added a pr flow to this PR and it's unhappy about something. I can't diagnose now, but I'll see what it doesn't like as soon as I can.

@wiedehopf
Copy link
Contributor

wiedehopf commented Jun 1, 2024

This seems to be showing errors for the state before your last commit.
Possibly because of when you requested the review?

Might need to re-request the review on the current state.

Anyhow your change looks good to me.

@fredclausen
Copy link
Member Author

Yeah I think the error was the original commit + my commit with the PR build action. The fix in the last commit seems to have fixed it. I'll do a local test build soon to make sure the build isn't broken before I merge in, but when I build it on my laptop it looks like it's doing everything right.

@wiedehopf
Copy link
Contributor

wiedehopf commented Jun 2, 2024

local build of this branch works fine.

mlat-client starts up ... all seems good :)

@fredclausen fredclausen merged commit 311e562 into main Jun 2, 2024
11 checks passed
@fredclausen
Copy link
Member Author

I'll merge it then :) Thanks @wiedehopf

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.

2 participants