.*: generate and embed Openshift's InstallConfig from Tectonic's Config #205
Conversation
openshift-ci-robot
added
approved
| region: eu-west-1 | ||
| vpcCIDRBlock: 10.0.0.0/16 | ||
| vpcID: "" | ||
| pullSecret: /path/config.json |
There was a problem hiding this comment.
This is no longer useful. The config should be updated to take the pull secret as a JSON string, even if users are allowed to provide it via a path.
There was a problem hiding this comment.
just to clarify, the types.InstallConfig.PullSecret field is a string type that stores the contents of the pull secret file , which is a JSON string?
There was a problem hiding this comment.
just to clarify, the
types.InstallConfig.PullSecretfield is a string type that stores the contents of the pull secret file , which is a JSON string?
That's fine with me. We expect to consume this as an opaque string, right? So we should store it as a string (like the SSH pubkey) instead of parsing it out into a more detailed structure.
abhinavdahiya
force-pushed
the
installconfig_in_old
branch
from
52e4027
to
e8813ae
Compare
| t.Errorf("error creating temp file: %v", err) | ||
| } | ||
| if _, err := tf.Write(pullSecretContents); err != nil { | ||
| t.Errorf("error writing to temp file %q: %v", tf.Name(), err) |
There was a problem hiding this comment.
To clarify my earlier comments, I don't think we want to store the pull secret path at all, and that we should replace PullSecretPath with a PullSecret string in the Cluster type. Then there would be no need for a temporary file here. The only filesystem connection would be on input, where we would provide users with the option to load the pull secret from a file. I set up something similar for SSH pubkeys here. For pull secrets, I expect the logic to be something like:
Pull secret:
Enter your pull secret as a single line of JSON (e.g. '{...}') or as a path to a file containing the pull secret JSON.
Then use an "is this valid JSON?" on the result. If it is, assume it's the pull secret. If not, assume it's the path and try to load valid JSON from the path. If you can't, complain about it and post the prompt again.
We have an existing "does this file contain valid JSON?" check here, but I think you'll want something like this to make it easy to get good invalid-JSON error messages for both strings and files.
There was a problem hiding this comment.
@wking
I'm sure we can change the Cluster type to store the pull secret. But i'm not convinced this the PR to change how we consume the pullsecret in Cluster
There was a problem hiding this comment.
I'm sure we can change the
Clustertype to store the pull secret. But i'm not convinced this the PR to change how we consume the pullsecret inCluster
I've filed #208 with the file -> string change. I'm fine landing that PR first and then rebasing this one on top (which would let you drop this temp file, etc.). I'm also fine landing this PR first and rebasing mine on top. Which way do you want to go?
There was a problem hiding this comment.
I can rebase on top of 208
abhinavdahiya
force-pushed
the
installconfig_in_old
branch
4 times, most recently
from
d760c28
to
937aa1b
Compare
|
@wking PTAL updated to use PullSecret string |
| @@ -95,6 +101,105 @@ func (c *ConfigGenerator) TectonicSystem() (string, error) { | |||
| }) | |||
| } | |||
|
|
|||
| // InstallConfig returns, if successful, a yaml string for on-disk install-config from 4.0 installer. | |||
There was a problem hiding this comment.
I'd reword to be more generic (no need to mention disks or 4.0). Maybe something like:
InstallConfig returns a YAML-rendered Kubernetes object with the user-supplied cluster configuration.
| PullSecret: c.PullSecret, | ||
| } | ||
|
|
||
| ic.Networking.Type = types.NetworkType(string(c.Networking.Type)) |
There was a problem hiding this comment.
nit: maybe move this into the ic literal above? Nothing in this line strikes me as something that couldn't happen there (e.g. no returned errors or other conditionals to deal with).
You could also move the podCIDR, serviceCIDR, and other setup to the top of this function, and then have a single:
return &types.InstallConfig{
...
}, nilat the end if they all succeed.
embedding the Openshift's InstallConfig in clusters created by tectonic installer binary allows operators to start building on their config discovery logic.
abhinavdahiya
force-pushed
the
installconfig_in_old
branch
from
937aa1b
to
f41cc55
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, wking 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 |
embedding the Openshift's InstallConfig in clusters created by tectonic
installer binary allows operators to start building on their config discovery
logic.