refactors logging to use annonation style #2546
Merged
Conversation
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
eks-distro-bot
added
do-not-merge/hold
size/L
jaxesn
force-pushed
the
jgw/cleaner-logging
branch
2 times, most recently
from
54808e5 to
a9d62ea
Compare
jaxesn
force-pushed
the
jgw/cleaner-logging
branch
from
a9d62ea to
8216155
Compare
jaxesn
commented
Oct 12, 2023
| @@ -840,7 +827,8 @@ release: $(RELEASE_TARGETS) | |||
| # Iterate over release branch versions, avoiding branches explicitly marked as skipped | |||
| .PHONY: %/release-branches/all | |||
| %/release-branches/all: | |||
| @for version in $(SUPPORTED_K8S_VERSIONS) ; do \ | |||
| @set -e; \ | |||
There was a problem hiding this comment.
on mac shellflags are not passed to the shell and missing this -e is annoying when running this locally since it ends up just continuing even if one version fails.
jaxesn
commented
Oct 12, 2023
| @@ -149,7 +152,7 @@ function build::gather_licenses() { | |||
| # data about each dependency to generate the amazon approved attribution.txt files | |||
| # go-deps is needed for module versions | |||
| # go-licenses are all the dependencies found from the module(s) that were passed in via patterns | |||
| build::common::echo_and_run go list -deps=true -json ./... | jq -s '' > "${outputdir}/attribution/go-deps.json" | |||
| build::common::echo_and_run go list -deps=true -json ./... | jq -s '.' > "${outputdir}/attribution/go-deps.json" | |||
There was a problem hiding this comment.
newer versions of jq do not like hte empty string. the . should be the same.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhay-krishna The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue #, if available:
Description of changes:
While working on improving the docker usage via the build targets, basically transparently using docker where necessary instead of relying on the eng to run the
in-dockertarget, I found an interesting (probably hacky) approach for adding target variables via a secondarily expanded prereq. In that case I am using this approach to dynamically add the docker target as a prereq to the real target and then overriding the shell totrueso that the on host command do not even run, since they ran in the docker container. This seems to work pretty well but I have a problem regarding how make outputs the commands before it runs them.While trying to solve the problem of outputting these commands, even tho they were not actually going to run, I was playing with this idea of using a bash script as the shell we could better control this output. This PR introduces an approach where instead of calling log_start and log_end in the recipe a secondarily expanded prereq is added, which just adds the shell override for the specific topic. Since this is secondarily expanded, the shell is not overridden until the target is actually be executed. This along with the new wildcard target which resets the SHELL to the default, makes it so that prereq targets do not inherit this shell override, which is the default for variables of targets with respect to that target's prereqs.
There are some cases where $(shell) calls are resolved during the generation + execution of recipe, in which case those will end up using the overridden SHELL, the bash script. In this case, we use an export variable, which only applies for the target given the enable_logging prereq to determine if we should log the pre and post log message.
The output of these targets should be same as before, as an example:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.