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

feat(KONFLUX-4136): new reduce-snapshot script #1968

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

scoheb
Copy link
Contributor

@scoheb scoheb commented Sep 17, 2024

  • add new script to ec-cli image

Copy link

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.11%. Comparing base (6625541) to head (fba5ae7).
Report is 14 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1968   +/-   ##
=======================================
  Coverage   74.11%   74.11%           
=======================================
  Files          88       88           
  Lines        5729     5729           
=======================================
  Hits         4246     4246           
  Misses       1483     1483           
Flag Coverage Δ
generative 74.11% <ø> (ø)
integration 74.11% <ø> (ø)
unit 74.11% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

hack/reduce-snapshot/reduce-snapshot.sh Outdated Show resolved Hide resolved
hack/reduce-snapshot/reduce-snapshot.sh Outdated Show resolved Hide resolved
hack/reduce-snapshot/reduce-snapshot.sh Outdated Show resolved Hide resolved
Comment on lines 51 to 52
SNAPSHOT=$(echo "$REDUCED_SNAPSHOT" | tr -d ' ' | tr -d '\n')
echo "$SNAPSHOT" > "${SNAPSHOT_PATH}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be dangerous if any of the keys or values in the JSON contain the space character, why not something like this:

Suggested change
SNAPSHOT=$(echo "$REDUCED_SNAPSHOT" | tr -d ' ' | tr -d '\n')
echo "$SNAPSHOT" > "${SNAPSHOT_PATH}"
echo "$SNAPSHOT" | jq -c . > "${SNAPSHOT_PATH}"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this now:

echo "$REDUCED_SNAPSHOT" | jq .
SNAPSHOT=$(echo "$REDUCED_SNAPSHOT" | tr -d ' ' | tr -d '\n')
echo "$SNAPSHOT" | jq -c . > "${SNAPSHOT_PATH}"

I think @zregvart meant this:

# for displaying
echo "$REDUCED_SNAPSHOT" | jq .
# for saving
echo "$REDUCED_SNAPSHOT" | jq -c . > "${SNAPSHOT_PATH}"

Honestly, I would just do something like this:

echo "$REDUCED_SNAPSHOT" | jq '.' | tee "${SNAPSHOT_PATH}"

I don't really think we need to save the reduced snapshot as a compacted json.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok adjusted. but I seem to remember an issue whereby I needed to remove the line feeds...

- add new script to ec-cli image

Signed-off-by: Scott Hebert <[email protected]>
@scoheb scoheb force-pushed the KONFLUX-4136-ec-cli-image branch from 433ef38 to 1e10232 Compare September 17, 2024 12:58
@scoheb scoheb requested a review from zregvart September 17, 2024 12:59
@lcarva
Copy link
Member

lcarva commented Sep 17, 2024

/ok-to-test

@lcarva lcarva enabled auto-merge September 17, 2024 15:16
@lcarva lcarva merged commit d50d943 into enterprise-contract:main Sep 17, 2024
13 checks passed
@@ -72,6 +72,9 @@ COPY --from=build "/build/dist/ec_${TARGETOS}_${TARGETARCH}" /usr/local/bin/ec
# Copy the one kubectl binary that can run in this container
COPY --from=build "/build/dist/kubectl_${TARGETOS}_${TARGETARCH}" /usr/local/bin/kubectl

# Copt reduce-snapshot script needed for single component mode
COPY hack/reduce-snapshot.sh /usr/local/bin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For tidiness I'd probably suggest:

COPY --from=build /build/hack/reduce-snapshot.sh /usr/local/bin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not super important, also I realize I'm too slow and its merged already.)

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.

4 participants