Skip to content
This repository has been archived by the owner on Mar 28, 2020. It is now read-only.

Add curl image parameter #2092

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

atiannicelli
Copy link

cluster: add option to specify curl image

add an option in EtcdCluster.spec:
CurlImage. Defaults to "tutum/curl:latest"

see #1933

Alex T. Iannicelli added 3 commits June 9, 2019 20:25
@etcd-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@etcd-bot
Copy link
Collaborator

Can one of the admins verify this patch?

1 similar comment
@etcd-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@alaypatel07 alaypatel07 modified the milestone: v0.9.5 Jun 25, 2019
@marcostvz
Copy link

hi @hasbro17, I need this feature too, could you or any other maintainer take a look? Thanks! :)

Copy link
Collaborator

@alaypatel07 alaypatel07 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. The CurlImage parameter is only consumed by the EtcdRestore cr and the PodPolicy is consumed by EtcdCluster.

IMO it is better to add it to the restore spec here

// curl init container image. default is tutum/curl.
// pulling tutum/curl:latest requires external access.
// More info: https://github.com/coreos/etcd-operator/issues/1933
CurlImage string `json:"curlImage,omitempty"`
Copy link
Collaborator

@alaypatel07 alaypatel07 Aug 30, 2019

Choose a reason for hiding this comment

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

Should be added to EtcdRestore struct

Copy link
Author

Choose a reason for hiding this comment

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

I am following the example of the "busyboxImage" parameter which specific to etcRestore as well. I see how you could say that it belongs there, but I feel these should stay together. THIS fix is to add the curlImage. If we want another fix to move them both to EtcdRestore I can see that working, but I would rather keep this as is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought the busyboxImage parameter is used for all etcd cluster pods and not just etcd restore

Image: imageNameBusybox(cs.Pod),

@vaneyckt
Copy link

vaneyckt commented Sep 17, 2019

hi @hasbro17, I just ran into this today as well. @atiannicelli do you need someone to take over from you on this?

@atiannicelli
Copy link
Author

We have been using this change for a few months and it works fine. I don't know how to "take it from here" as I don't have merge ability and it seems that the tests are not being run above. I don't know who to contact to get an approving review from someone with write access.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants