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

*: remove support for environment variables #861

Merged
merged 3 commits into from
Dec 12, 2018
Merged

*: remove support for environment variables #861

merged 3 commits into from
Dec 12, 2018

Conversation

crawford
Copy link
Contributor

@crawford crawford commented Dec 10, 2018

The environment variables were originally added to make CI testing a
little easier, since the installer didn't support consumption of
provided assets (e.g. the install config). Now that the installer
supports consumption, there is no need for most of the environment
variables anymore. The variables have actually been confusing to users,
so their removal should simplify the mental model.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 10, 2018
@crawford
Copy link
Contributor Author

Requires openshift/release#2353.

docs/dev/libvirt-howto.md Outdated Show resolved Hide resolved
pkg/asset/userprovided.go Outdated Show resolved Hide resolved
@wking
Copy link
Member

wking commented Dec 11, 2018

How are we planning on rolling this out? The gentlest way (at this point in our unstable development) would probably be to drop the docs in one PR, and then (after giving folks a week to react to the change-log's deprecation notice) to drop the implementation in the next release. Or we can land them both in one PR? Now that we're cutting frequent, pinned release, I'm comfortable with either. Folks who run master should read the Git history, and folks who run pinned releases can see the API change in the change-log and decide whether or not they can update. The only issue with that would be the BYOR folks who got bit by #644 (so CC @vrutkovs about this change) and are probably floating on master without reading our Git history ;).

@michaelgugino
Copy link
Contributor

I think if you're going to remove the environment variables you should also remove the prompts altogether. Just give people an example install-config to fill out. Dynamically generating a file just to turn around and have to edit it is burdensome and my suspicion is that the install-config file will be lacking proper documentation because 'you really shouldn't mess with it anyway' kind of thing.

@crawford
Copy link
Contributor Author

I think if you're going to remove the environment variables you should also remove the prompts altogether. Just give people an example install-config to fill out.

We still want the prompts for those users which don't want to do any customization and just want to get a cluster up and running as quickly as possible.

Dynamically generating a file just to turn around and have to edit it is burdensome and my suspicion is that the install-config file will be lacking proper documentation because 'you really shouldn't mess with it anyway' kind of thing.

The install-config is the thing we want people messing with though. We just haven't finalized it's shape and therefore haven't spent much time on documentation. The multiple-invocation nature of the installer is central to providing escape hatches at many different levels. Yes, it may be burdensome, but that is the price you pay for deeper customization. We don't want to drag everyone down into the weeds because a few users need to heavily customize their setup.

@crawford
Copy link
Contributor Author

How are we planning on rolling this out?

I want to get this out as quickly as possible before too many people become reliant on them. I'm fine dumping it into the next release without any deprecation period.

The environment variables were originally added to make CI testing a
little easier, since the installer didn't support consumption of
provided assets (e.g. the install config). Now that the installer
supports consumption, there is no need for most of the environment
variables anymore. The variables have actually been confusing to users,
so their removal should simplify the mental model.
@crawford crawford changed the title WIP: *: remove support for environment variables *: remove support for environment variables Dec 11, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2018
Now that support for environment variables has been removed, there isn't
much of a need for the GenerateUserProvidedAsset wrapper function.
@abhinavdahiya
Copy link
Contributor

This is 💯
/approve

This just shows how to reuse an install config, but more will be added
to this doc over time.
@ironcladlou
Copy link
Contributor

lgtm, I'm okay ripping off the bandaid and getting to work on environment-less workflows.

@vrutkovs
Copy link
Member

/retest

vrutkovs added a commit to vrutkovs/release that referenced this pull request Dec 12, 2018
The change should first land in the installer, see 
openshift/installer#861
@crawford
Copy link
Contributor Author

Error executing openshift-install: waiting for bootstrap-complete: timed out waiting for the condition

This has been killing us lately.

@crawford
Copy link
Contributor Author

Error executing openshift-install: waiting for bootstrap-complete: timed out waiting for the condition

Hopefully openshift/cluster-openshift-apiserver-operator#91 helps.

/retest

@crawford
Copy link
Contributor Author

aws_instance.master.0: Error launching source instance: timeout while waiting for state to become 'success' (timeout: 30s)

Come on AWS...

/pray
/retest

@crawford
Copy link
Contributor Author

This error appears to be caused by "Server.InsufficientInstanceCapacity" on AWS's end. We might need to switch off of t3.medium until AWS installs more hardware (cc @wking).

@crawford
Copy link
Contributor Author

We currently do not have sufficient t3.medium capacity in the Availability Zone you requested (us-east-1a). Our system will be working on provisioning additional capacity. You can currently get t3.medium capacity by not specifying an Availability Zone in your request or choosing us-east-1d, us-east-1b, us-east-1c, us-east-1f.

@vrutkovs
Copy link
Member

/retest

@wking
Copy link
Member

wking commented Dec 12, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, crawford, 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:
  • OWNERS [abhinavdahiya,crawford,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@alejovicu
Copy link
Contributor

Maybe the documentation of SSH Access in docs/dev/libvirt-howto.md should be updated to remove the reference to the envars:
OPENSHIFT_INSTALL_CLUSTER_NAME
OPENSHIFT_INSTALL_BASE_DOMAIN
OPENSHIFT_INSTALL_LIBVIRT_URI

@openshift-merge-robot openshift-merge-robot merged commit 897fcdd into openshift:master Dec 12, 2018
@crawford crawford deleted the env-vars branch December 13, 2018 00:08
@wking
Copy link
Member

wking commented Dec 13, 2018

@alejovicu, file a follow-up?

@ghost
Copy link

ghost commented Dec 13, 2018

@wking
So after this, how can we do things with CI? Is it still supported?

wking added a commit to wking/openshift-installer that referenced this pull request Dec 13, 2018
The long forms are less likely to exist in the user's environment
since 6be4c25 (*: remove support for environment variables,
2018-12-10, openshift#861), and we no longer need the context to distinguish
from all the other environment variables on a user's system.
@wking
Copy link
Member

wking commented Dec 13, 2018

So after this, how can we do things with CI?

The CI updates happend in openshift/release#2353. Did you need something besides that?

@ghost
Copy link

ghost commented Dec 13, 2018

@wking Got you, means we have to make install-config.yaml as a template and generate target file via a template system which maybe build in ansible or other things, or draft one.

@gpei
Copy link
Contributor

gpei commented Dec 13, 2018

@wking Got you, means we have to make install-config.yaml as a template and generate target file via a template system which maybe build in ansible or other things, or draft one.

yeah, like this Jinja2 template file https://github.com/vrutkovs/openshift-40-centos/blob/master/install-config.yml.j2

But one more concern is that the template is still a static file, once some changes made in new generated install-config.yaml, we need to apply corresponding updates to our template as well. Especially in the future, customers may have more customized requests to the installer. So we may always have to update the template, it still will be a challenge for CI jobs IMO.

cgwalters added a commit to cgwalters/homegit that referenced this pull request Dec 13, 2018
@crawford
Copy link
Contributor Author

once some changes made in new generated install-config.yaml, we need to apply corresponding updates to our template as well

This is true of any input, including environment variables. As we added, removed, and renamed variables, CI had to be kept up-to-date just the same. As developers make changes here, they will be responsible for making the necessary changes to CI.

wking added a commit to wking/openshift-installer that referenced this pull request Dec 18, 2018
Catching up with 6be4c25 (*: remove support for environment
variables, 2018-12-10, openshift#861).

Also link to the troubleshooting docs, since the easier it is to
discover those, the fewer bug reports we'll get with insufficient
detail ;).
wking added a commit to wking/openshift-the-easy-way that referenced this pull request Jan 4, 2019
A few changes:

* Backticks for some code like ~/Downloads/pull-secret.  This change
  is not exhaustive.
* Drop environment variables to catch up with
  openshift/installer@6be4c253 (*: remove support for environment
  variables, 2018-12-10, openshift/installer#861).
* Link to the installer repository for a list of supported platforms.
* Link to the installer repository for discussion of repeat calls,
  since that seems useful, but out-of-scope for a workshop-length
  introduction.
wking added a commit to wking/hive that referenced this pull request Feb 5, 2019
Take advantage of openshift/origin@59dab63d (Temporarily force the
installer to be part of the payload, 2018-12-08,
openshift/origin#21637) to avoid installer/update-payload mismatches.
This does not address install-config.yaml/installer mismatches, but
it's a step in the right direction.

Also:

* Replace 'shell' syntax highlighting with 'console', because these
  blocks have prompts.  And switch from the traditionally-PS2 '>' to
  the traditionally-PS1 '$' for those prompts.
* Stop exporting variables that are not needed by subprocesses.
* Drop OPENSHIFT_INSTALL_*.  This information is provided via
  install-config.yaml, and the installer ignores the environment
  variables since openshift/installer@6be4c253 (*: remove support for
  environment variables, 2018-12-10, openshift/installer#861).

There's an outstanding FIXME while I wait for guidance about who's job
it is to pull the installer image out of the update payload.  If we do
that in generate.go, we need to vendor some
not-really-designed-as-a-library origin code.  If we do it in the
launched container, we need both oc and Podman (or some other way to
actually run the discovered installer image).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants