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

chore: Remove legacy output metadata annotations #1671

Merged
merged 2 commits into from Mar 9, 2020

Conversation

@elliott-davis
Copy link
Contributor

elliott-davis commented Mar 5, 2020

What this PR does / why we need it:

Annotations were being set with the stdout of tasks run with services. Tasks with large amounts of stdout were causing their dependent services to fail if they hit the character limit for k8s annotations.

Special notes for reviewer
I was having one test fail locally but it seems unrelated to my change. If anything stands out and it fails in CI, please let me know and I'll get it fixed up.

}

// Make the full list of dependencies and outputs available as JSON as well
result.envVars.GARDEN_DEPENDENCIES = JSON.stringify(result.dependencies)

This comment has been minimized.

Copy link
@edvald

edvald Mar 5, 2020

Collaborator

I'm actually wondering if we should keep this one, but omit the outputs key on each dependency. I've at least used it once myself, and it's only the outputs that we want to get rid of here.

This comment has been minimized.

Copy link
@elliott-davis

elliott-davis Mar 5, 2020

Author Contributor

I left it initially but the service I was running crashed on startup because the log output of the task ended up in the GARDEN_DEPENDENCIES envvar. It succeeded on subsequent runs where the task didn't execute. I'm happy to leave it in and maybe add some sanitization around what outputs can pass through?

This comment has been minimized.

Copy link
@edvald

edvald Mar 5, 2020

Collaborator

I'd suggest removing the outputs field from every dependency, keep it simple. Something like JSON.stringify(result.dependencies.map((d) => { ...d, outputs: undefined }).

This comment has been minimized.

Copy link
@elliott-davis

elliott-davis Mar 5, 2020

Author Contributor

Didn't exactly work like that (I couldn't get the spread operator to cooperate) but it's close and I didn't have to use Object.assign

This comment has been minimized.

Copy link
@swist

swist Mar 5, 2020

Contributor

Just need extra parens:

JSON.stringify(result.dependencies.map((d) =>( { ...d, outputs: undefined }))

This comment has been minimized.

Copy link
@edvald

edvald Mar 5, 2020

Collaborator

Oops, sorry 😬

This comment has been minimized.

Copy link
@elliott-davis

elliott-davis Mar 6, 2020

Author Contributor

Shows how long I've been out of the javascript game - thanks @swist

@edvald

This comment has been minimized.

Copy link
Collaborator

edvald commented Mar 5, 2020

Thanks @elliott-davis :) One comment, otherwise looks good!

@elliott-davis elliott-davis force-pushed the elliott-davis:elliott/annotation_cleanup branch from 513af8d to 940e2ef Mar 5, 2020
@elliott-davis elliott-davis force-pushed the elliott-davis:elliott/annotation_cleanup branch from 940e2ef to 7d906d1 Mar 6, 2020
@edvald

This comment has been minimized.

Copy link
Collaborator

edvald commented Mar 6, 2020

One last comment + a couple of small linting errors. Otherwise 👍 👍

@elliott-davis elliott-davis force-pushed the elliott-davis:elliott/annotation_cleanup branch 2 times, most recently from 4488348 to 637fcd1 Mar 6, 2020
Annotations were being set with the stdout of tasks run with services. Tasks with large amounts of stdout were causing their dependent services to fail if they hit the character limit for k8s annotations.
@elliott-davis elliott-davis force-pushed the elliott-davis:elliott/annotation_cleanup branch from 637fcd1 to c14a04b Mar 6, 2020
@edvald
edvald approved these changes Mar 9, 2020
@edvald edvald merged commit f43629b into garden-io:master Mar 9, 2020
6 checks passed
6 checks passed
commit Workflow: commit
Details
test-minikube-1.12 Workflow: test-minikube-1.12
Details
test-minikube-1.13 Workflow: test-minikube-1.13
Details
test-minikube-1.14 Workflow: test-minikube-1.14
Details
test-minikube-1.15 Workflow: test-minikube-1.15
Details
test-minikube-1.16 Workflow: test-minikube-1.16
Details
@elliott-davis elliott-davis deleted the elliott-davis:elliott/annotation_cleanup branch Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.