-
-
Notifications
You must be signed in to change notification settings - Fork 541
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
fix(docker): Prevent all possible "silent errors" during docker build
#644
Merged
+409
−192
Merged
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
4e6a011
fix(docker): Prevent all possible "silent errors" during `docker build`
MaxymVlasov 5de5a97
ci: Fix platform
MaxymVlasov 82cb162
Add QEMU wich hope that it will change anything
MaxymVlasov 35f89ba
Apply suggestions from code review
MaxymVlasov 0749051
Apply revie suggestions
MaxymVlasov 6ebe879
Templating and move out most common part
MaxymVlasov eed0722
Use one function to install GH releases
MaxymVlasov cb59613
Apply review suggestions
MaxymVlasov 65ba593
Fix "redefinition" of global vars in function
MaxymVlasov 133fe03
Apply suggestions from code review
MaxymVlasov 0fbbb5b
Rewrite `readonly` definitions + fix dumb error
MaxymVlasov 51cfcd9
Minor style improvements
MaxymVlasov 8c86ac0
Fix tests for arm
MaxymVlasov 84b0433
fix refactoring error
MaxymVlasov 8c06a41
Merge branch 'master' into docker/improve_error_handling
MaxymVlasov fd6efa8
Apply review suggestions
MaxymVlasov b3436cf
Reorder fields
MaxymVlasov 6b0c4ee
Add example of how to add add a new dependency
MaxymVlasov 8f1e05c
nfracost changed their version output
MaxymVlasov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix "redefinition" of global vars in function
commit 65ba59343a0faf47e761ac55e58495a9bc34b8cf
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Maybe re-use
DISTRIBUTED_AS
?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.
Nope, because:
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.
The
binary
types just don't use theDISTRIBUTED_AS
inGH_RELEASE_REGEX_LATEST
andGH_RELEASE_REGEX_SPECIFIC_VERSION
vars so they are not affected at all by the suggested change.This is probably about something else.
I see the code:
and
and I see no reason to not swap these two to the below:
and
No change to
binary
types and optimization tonon-binary
types.Am I missing something non-obvious?
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.
Once again:
Imagine that you adding a new hook, let's say tfupdate.
You copy-past file, IE
tfsec.sh
rename it, cleanup pre-populated values and get this:How you should know
DISTRIBUTED_AS
value till you don't get any ofGH_RELEASE_...
?Please, don't make it more complex than it already is.
Also, someone could distribute not
.tar.gz
but.tgz
.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.
From what I see in each new script for each tool installation, the value of
DISTRIBUTED_AS
is known and is defined in each of these files. Along with that most of the files have the value ofDISTRIBUTED_AS
repeated three times. And my suggestion is to reduce it to one time.I'm trying to simplify so that new tools are added without a need to copy&paste
DISTRIBUTED_AS
value three times on a three lines in a row.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.
Read once again, maybe tomorrow
#644 (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.
You're wrong, unless you prove opposite 🤝
That comment makes no sense to me. Re-write it once again, maybe tomorrow 😏
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.
There is a process of adding a new tool from scratch
Screencast from 24.04.24 21:01:35.webm
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.
DISTRIBUTED_AS
value based on URLAlso, I see that I'm already stuck to recreate that process from memory for LATEST regex, which means that this flow is already complicated.
That verbosity does not hurt anyone.
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.
Version without stuck (will add it to contributor docs)
Screencast.from.24.04.24.22.00.36.webm