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

Enable XRay by default #255

Merged
merged 1 commit into from
Jan 19, 2021
Merged

Enable XRay by default #255

merged 1 commit into from
Jan 19, 2021

Conversation

roborourke
Copy link
Contributor

This was causing problem with the Cloud module's default setting fro XRay so we should avoid errors with a vanilla install. Devs will need to switch off both the cloud module setting and the local server setting to fully turn off xray locally.

Fixes #254

This was causing problem with the Cloud module's default setting fro XRay so we should avoid errors with a vanilla install. Devs will need to switch off both the cloud module setting and the local server setting to fully turn off xray locally.

Fixes #254
Copy link
Contributor

@jazzsequence jazzsequence left a comment

Choose a reason for hiding this comment

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

This change is fine. However, before closing out #254 -- and given that it's indicated in the notes -- I would request that there be documentation added as part of #254 that explains how to disable Xray locally (and possibly why). Since the fixed keyword is used in the PR, merging this will auto-close that issue and I feel like there should be a separate, documentation component to this, hence requesting changes.

@roborourke
Copy link
Contributor Author

@jazzsequence we haven't documented any of the new config options for local server and the containers you can turn off yet because the feature isn't really complete. The other modules that would be affected by turning off say the ES container don't do any checks against the Local Server config so I'm not sure we're quite there yet.

Are you asking for a new issue to be created for the docs or for all the docs to be added to this PR? I'd rather do them all together when we're ready to. I'll create a new issue for that so that this change doesn't need to be held up.

@roborourke
Copy link
Contributor Author

Created #257 as a follow up to document the options.

Copy link
Contributor

@jazzsequence jazzsequence left a comment

Choose a reason for hiding this comment

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

Are you asking for a new issue to be created for the docs or for all the docs to be added to this PR?

I was specifically asking for documentation about turning Xray off, specifically, since it's mentioned in the PR description ("Devs will need to switch off both the cloud module setting and the local server setting to fully turn off xray locally.") -- if devs are expected to know a thing, the thing should be documented.

However, if the feature is as yet unannounced, and there's more associated documentation needed to cover all the aspects, and we have an issue for that, that's enough for me to have the ticket and toggle the default back to true.

@jazzsequence jazzsequence merged commit a65ca5d into master Jan 19, 2021
@jazzsequence jazzsequence deleted the enable-xray branch January 19, 2021 20:33
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.

XRay container is off by default but Cloud module stills loads XRay code
2 participants