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

Explain the customCheProperties field #838

Merged
merged 6 commits into from
Oct 7, 2019

Conversation

tomgeorge
Copy link
Contributor

@tomgeorge tomgeorge commented Oct 3, 2019

What does this PR do?

Explains the new overrideCheProperties field in the CheCluster Custom Resource and shows an example CR that has an additional property.

What issues does this PR fix or reference?

eclipse-che/che#14635

@tomgeorge
Copy link
Contributor Author

I think we are going to change the field name in the CheCluster cr, so these docs will need to change.

Copy link
Contributor

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

Please look at my comments

@tomgeorge
Copy link
Contributor Author

@rkratky @davidfestal I made the changes that you suggested.

Signed-off-by: Tom George <[email protected]>
@tomgeorge
Copy link
Contributor Author

@rkratky we are ready with eclipse-che/che-operator#87. Once this is approved we will merge

Copy link
Contributor

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

All is OK on my side.
Let's just wait for docs team approval before merging the main PR.

Copy link
Contributor

@nickboldt nickboldt left a comment

Choose a reason for hiding this comment

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

Instead of FOO: BAR why not include some (or all) of the image and pull_policy references in che.properties, so we have an example of WHY you'd use this override mechanism?

eg. for productization and using chectl to deploy CRW:

CHE_SERVER_SECURE__EXPOSER_JWTPROXY_IMAGE: 'registry.redhat.io/codeready-workspace/jwtproxy-rhel8:2.0'

and for airgap:
CHE_FACTORY_DEFAULT__PLUGINS: 'myquay.mycorp.com/che-machine-exec-plugin/7.2.7'
CHE_WORKSPACE_SIDECAR_IMAGE__PULL__POLICY: 'Never'

I would also link to https://github.com/eclipse/che/blob/master/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties and explicitly explain how those properties map to fields in a yaml file:

  • uppercase the parameter
  • replace . with _ and _ with __

Thus:

che.workspace.plugin_broker.pull_policy -> CHE_WORKSPACE_PLUGIN__BROKER_PULL__POLICY

@davidfestal
Copy link
Contributor

I would also link to https://github.com/eclipse/che/blob/master/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties and explicitly explain how those properties map to fields in a yaml file:

* uppercase the parameter

* replace `.` with `_` and `_` with `__`

Thus:

che.workspace.plugin_broker.pull_policy -> CHE_WORKSPACE_PLUGIN__BROKER_PULL__POLICY

Seems to me that this is more related to a different docs issue:

eclipse-che/che#13886 (comment)

I'd rather not block the merge of PR eclipse-che/che#14635 for this.

@davidfestal
Copy link
Contributor

eg. for productization and using chectl to deploy CRW:

CHE_SERVER_SECURE__EXPOSER_JWTPROXY_IMAGE: 'registry.redhat.io/codeready-workspace/jwtproxy-rhel8:2.0'

and for airgap:
CHE_FACTORY_DEFAULT__PLUGINS: 'myquay.mycorp.com/che-machine-exec-plugin/7.2.7'
CHE_WORKSPACE_SIDECAR_IMAGE__PULL__POLICY: 'Never'

Maybe I would keep the air-gap aspect for this other Che docs PR : eclipse-che/che#14088 (comment)

Copy link
Contributor

@nickboldt nickboldt left a comment

Choose a reason for hiding this comment

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

Ok, @davidfestal those other issues can cover the doc issues.

But I'd still rather see a real(ish) example than FOO:BAR like

CHE_SERVER_SECURE__EXPOSER_JWTPROXY_IMAGE: 'registry.redhat.io/codeready-workspace/jwtproxy-rhel8:2.0'

@tomgeorge tomgeorge requested a review from nickboldt October 4, 2019 20:26
@themr0c themr0c dismissed nickboldt’s stale review October 7, 2019 08:37

Requested change applied in the last commit.

@themr0c themr0c merged commit fdeb46e into eclipse-che:master Oct 7, 2019
@davidfestal
Copy link
Contributor

@themr0c why did you merge this already ? Shouldn't we wait for the corresponding main PR to be merged to master and, even more, released to production with the 7.3.0 release ?

@themr0c
Copy link
Contributor

themr0c commented Oct 7, 2019

How should I have known?

@davidfestal
Copy link
Contributor

How should I have known?

I thought it was the usual way: only merge a doc change when the related product change is released.
But I should have been clearer in the PR description it seems.

@themr0c
Copy link
Contributor

themr0c commented Oct 7, 2019

Yes please prepend "[WIP]" to the title of your PR when it should not me merged, and remove it once it can be merged.

It's first time we are ahead of the product, documentation is usually lagging behind, so I didn't even thought about verifying the feature is present!

@nickboldt nickboldt changed the title Explain the overrideCheProperties field Explain the customCheProperties field Oct 15, 2019
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

Successfully merging this pull request may close these issues.

6 participants