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

service update #12

Closed
sixolet opened this issue Jan 29, 2019 · 12 comments
Closed

service update #12

sixolet opened this issue Jan 29, 2019 · 12 comments
Assignees
Milestone

Comments

@sixolet
Copy link
Contributor

sixolet commented Jan 29, 2019

Write the command to update a service! It should never change the code you are running unless you specifically specify a new image/build.

@sixolet sixolet self-assigned this Jan 29, 2019
@csantanapr
Copy link
Member

/milestone v0.1.0

@knative-prow-robot
Copy link
Contributor

@csantanapr: You must be a member of the knative/knative-milestone-maintainers github team to set the milestone.

In response to this:

/milestone v0.1.0

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@csantanapr csantanapr added this to the v0.1.0 milestone Apr 5, 2019
@maximilien
Copy link
Contributor

OK, so maybe this one can be my next victim :)

@sixolet are you thinking update for all params but the image, so for instance:

kn service update old-name --name new-name --request-cpu 1000m --limits-memory 1024Mi

This would update the service's name to new-name and any values in the request CPU and limits memory as specified.

kn service update new-name --env NEW_KEY=NEW_VAL KEY=NEW_VAL --limits-cpu 1000m

This would update the service with new-name with a new environment variable NEW_KEY and update the old environment variable KEY with NEW_VAL

And so on.

@rhuss
Copy link
Contributor

rhuss commented Apr 24, 2019

name is immutable, so for an update, you would have to delete and create the service. This is true for Kubernetes resources in general.

Do we have (or is it planned) to have a rollout or deploy command for updating the image ? If so, I agree to no allow to change the image here. Otherwise changing the image via an update is an easy way to trigger a redeployment.

For an analogy to kubectl: kubectl uses kubectl set image or kubectl set env etc. to change specific fields.

I'm not sure what would be nicer in our context:

kn service set env ....  # all possible options that 'kubectl set env' supports
kn service set image ...
kn service set ...

or

kn service update --image=... --env ....

I think the former is more straight forward if you want to support multiple ways for e.g. updating env vars (like from a file, or managing multiple env, or other stuff). Interestingly there in kubectl set env there are also very ugly things like kubectl set env pods --all --list for only listing env without changing them. We shouldn't copy that here :)

@maximilien
Copy link
Contributor

maximilien commented Apr 24, 2019

I like kn service update --image=... --env .... reads better (English) and results in less commands.

But happy to do whichever. At this point just want to move forward.

cc: @sixolet and @cppforlife what say you? I want to get started on this today or tomorrow. Please provide your input please. No input tomorrow then I start with @rhuss suggestion. We can always change later if a strong opinion emerges.

@rhuss
Copy link
Contributor

rhuss commented Apr 25, 2019

@maximilien it really depends how much power you want to add for changing e.g. env.

If it's ok to update single env vars only (potentially with the option to remove/add one, too, then I agree doing it per options makes is more lighweight.

If we want to support all env var manipulating features of kubectl like:

  • Specifying env literally
  • Reading env vars from a file
  • Using a configmap / secrret for the env var

then a dedicated subcommand could make more sense to not overwhelm the number of options required.

@maximilien
Copy link
Contributor

maximilien commented Apr 25, 2019

OK, @rhuss I get your point. However, I would vote for one command and allowing multiple flags as needed. Nothing prevents having multiple --env ... on a request?

And if more env manipulations are needed then we can explore special command.

@evankanderson
Copy link
Member

Not sure if this will apply here, but I have seen situations in the past where multiple changes needed to be done together, or at least in a specified sequence. To pick on an example that makes me sad every time:

kn service update kaffe --request-mem 1024Mb --env JAVA_XMX=700M

(assuming that the actual args to java -jar foo.jar includes an -Xmx${JAVA_XMX} flag)

Other items that can vary similarly:

  • ServiceAccount and Env Vars (for repointing to another database)
  • Concurrency / Timeout and args

@cppforlife
Copy link

my vote is for having additional flags on update command.

@maximilien
Copy link
Contributor

OK I'll move forward with kn service update --image=... --env .... and monitor here for additional opinions or comments. Cheers 🍻

@rhuss
Copy link
Contributor

rhuss commented Apr 26, 2019

Not sure if this will apply here, but I have seen situations in the past where multiple changes needed to be done together, or at least in a specified sequence.

If you require the changes applied in a specific sequence, using options means to imply an order on the options given. Not sure if you can easily find that out with the current libraries we are using for option handling (and its a bad UX anyway, as no one else imposes semantics on an option ordering. Arguments would be used for that).

I agreed that its good for situations where you want to update multiples aspects atomically.

So I'm fine with options as long as we don't impose an option order but apply all changes with a single update call.

wrt/ to env: We should restrict ourselves here to support literal env vars (no configmap/secret refs) and support multiple options given with --env to update multiple envs at once. Also supporting the syntax --env TOREMOVENV- to remove envs (i.e. a trailing - indicates the removal of an env, like kubectl set env does) would be awesome.

@csantanapr
Copy link
Member

What type of env vars was discussed in one of the WG meetings when defining the MVP for the first release and group agree to keep it simple and implement first only simple strings for env vars

@sixolet sixolet closed this as completed May 14, 2019
coryrc pushed a commit to coryrc/client that referenced this issue May 14, 2020
…tive#12)

* Initial version of presubmit-tests.sh helper in prow-tests image

This helper manages flag parsing and presubmit skipping, leaving the real script in the repos with only the important stuff (i.e., running the tests themselves).

This will reduce boilerplate code and unify the testing strategy and features for presubmits across ther repos.

* Reorganize source and clarify comments.
coryrc pushed a commit to coryrc/client that referenced this issue May 14, 2020
We're consolidating the test infrastructure into a single place, so all repos get the same fixes, updates and new features.

`presubmit-tests.sh` helper was implemented by knative#12
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

No branches or pull requests

7 participants