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

[Monitoring] Ensure cloud cannot see setup mode #49223

Merged

Conversation

chrisronline
Copy link
Contributor

@chrisronline chrisronline commented Oct 24, 2019

Resolves #49225

To test, pull down this PR and manually ensure isOnCloud is true here

@chrisronline chrisronline changed the title Ensure cloud cannot see setup mode [Monitoring] Ensure cloud cannot see setup mode Oct 24, 2019
@chrisronline chrisronline marked this pull request as ready for review October 24, 2019 17:24
Copy link
Contributor

@igoristic igoristic left a comment

Choose a reason for hiding this comment

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

LGTM Great job!

@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring (Team:Monitoring)

@chrisronline
Copy link
Contributor Author

@igoristic Would love any feedback on how to make 72b8a67 better :(

Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

Tested this per the instructions and works as promised!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@igoristic
Copy link
Contributor

@igoristic Would love any feedback on how to make 72b8a67 better :(

@chrisronline
Unfortunately this is just the nature of it 😞 The only thing I would change (more of a preference/nit) is move the ui/chrome mock into setModules(), and also call clearAllMocks() just in case. It would look something like this:

function setModules(isOnCloud = false) {
  
  jest.clearAllMocks().resetModules();
  injectorModulesMock.globalState.inSetupMode = false;

  jest.doMock('ui/chrome', () => ({
    getInjected: (key) => {
      if (key === 'isOnCloud') {
        return isOnCloud;
      }
    }
  }));

  const setupMode = require('./setup_mode');
  toggleSetupMode = setupMode.toggleSetupMode;
  initSetupModeState = setupMode.initSetupModeState;
  getSetupModeState = setupMode.getSetupModeState;
  updateSetupModeData = setupMode.updateSetupModeData;
  setSetupModeMenuItem = setupMode.setSetupModeMenuItem;
}

So, now instead of this: https://github.com/elastic/kibana/pull/49223/files#diff-16124668cbf2ae94c6eafa83ae18894fL112-R133 I would just call setModules(true)

@chrisronline
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

chrisronline added a commit to chrisronline/kibana that referenced this pull request Nov 4, 2019
* Ensure cloud cannot see setup mode

* Remove cloud check from collection status, as it's an injected var now

* Man these tests suck
chrisronline added a commit that referenced this pull request Nov 5, 2019
* Ensure cloud cannot see setup mode

* Remove cloud check from collection status, as it's an injected var now

* Man these tests suck
chrisronline added a commit that referenced this pull request Nov 5, 2019
* Ensure cloud cannot see setup mode

* Remove cloud check from collection status, as it's an injected var now

* Man these tests suck
@chrisronline
Copy link
Contributor Author

Backport:
7.x: dd43b7d
7.5: a615e8c

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

Successfully merging this pull request may close these issues.

Monitoring - Enter set up mode link should not be featured on cloud deployments.
4 participants