Use install properties to check if triggers is installed

[eddycharly]

Changes

This PR changes the way we detect if triggers is installed.
It uses the getInstallProperties api instead of trying to access the CRD.

/cc @a-roberts @AlanGreene

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices

See the contribution guide
for more details.

[AlanGreene]

The install properties are already available in the redux store (fetched in App.js) so there should be no need to request them again here.

We could just add an isTriggersInstalled (or similar) selector in https://github.com/tektoncd/dashboard/blob/master/src/reducers/properties.js and use that in the SideNav.

[eddycharly]

The install properties are already available in the redux store (fetched in App.js) so there should be no need to request them again here.

Sorry, i didn't know that, I just borrowed the code from the About page.

Time to learn about reducers i guess 😂

[eddycharly]

/test tekton-dashboard-unit-tests

[eddycharly]

I don't understand why tests are failing, any help on this would be nice ! 🙏

[AlanGreene]

Taking a look

[AlanGreene]

OK so in the SideNav test you're mocking both isReadOnly and isTriggersInstalled, these will apply to all tests in the file.

If you want to override the behaviour for a specific test (e.g. the 'SideNav renders with triggers' test below), you can do this:

beforeEach(() => {
  jest.spyOn(selectors, 'isReadOnly').mockImplementation(() => true);
  jest.spyOn(selectors, 'isTriggersInstalled').mockImplementation(() => false);
});

...

it('SideNav renders with triggers', async () => {
  selectors.isTriggersInstalled.mockImplementation(() => true);

Calling jest.spyOn on an existing spy to update its behaviour may work in some scenarios, but it's generally safer to do it as <existingSpy>.mockImplementation(...) instead.

I'm not sure why the error message wasn't helpful this time, they're usually much better...

[eddycharly]

Thank you, I will test this tomorrow !

[eddycharly]

No luck.

Test SideNav selects namespace based on URL for example raises the error TypeError: Cannot read property 'props' of undefined

I might be doing something wrong but dunno what :(

[AlanGreene]

Strange, I'll take another look at this later today and see what I can come up with.

[AlanGreene]

There's something strange going on with the SideNavMenu component in Carbon. I've reached out to the devs to see if it's a bug or if perhaps we're using the component in an unexpected way and there's something else we can do to resolve / work around this.

In the meantime, adding defaultProps on the SideNav component does the trick:

diff --git a/src/containers/SideNav/SideNav.js b/src/containers/SideNav/SideNav.js
index c435667..3f71868 100644
--- a/src/containers/SideNav/SideNav.js
+++ b/src/containers/SideNav/SideNav.js
@@ -338,6 +338,10 @@ class SideNav extends Component {
   }
 }
 
+SideNav.defaultProps = {
+  isTriggersInstalled: false
+};
+
 /* istanbul ignore next */
 const mapStateToProps = state => ({
   extensions: getExtensions(state),
diff --git a/src/containers/SideNav/SideNav.test.js b/src/containers/SideNav/SideNav.test.js
index e056716..ed33d34 100644
--- a/src/containers/SideNav/SideNav.test.js
+++ b/src/containers/SideNav/SideNav.test.js
@@ -62,7 +62,8 @@ it('SideNav renders with extensions', () => {
 });
 
 it('SideNav renders with triggers', async () => {
-  jest.spyOn(selectors, 'isReadOnly').mockImplementation(() => false);
+  selectors.isReadOnly.mockImplementation(() => false);
+  selectors.isTriggersInstalled.mockImplementation(() => true);
 
   const middleware = [thunk];
   const mockStore = configureStore(middleware);
@@ -586,7 +587,7 @@ it('SideNav updates namespace in URL', async () => {
 });
 
 it('SideNav renders import in not read-only mode', async () => {
-  jest.spyOn(selectors, 'isReadOnly').mockImplementation(() => false);
+  selectors.isReadOnly.mockImplementation(() => false);
 
   const middleware = [thunk];
   const mockStore = configureStore(middleware);

[eddycharly]

Thank you so much ! 😅

[eddycharly]

@a-roberts @AlanGreene i think this one should be pretty safe to merge also

[a-roberts]

Yeah, so the jist of it being

export function isTriggersInstalled(state) {
  return (state.TriggersNamespace && state.TriggersVersion) || false;
}

I see, nice compact way of doing it, those detection mechanisms are already good on OpenShift so let's get it in unless @AlanGreene has any objections (holding the lgtm)

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-roberts

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 [a-roberts]

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

[AlanGreene]

Yeah looks good to me, tested it out locally and working nicely. I need to think a bit about how it affects #1388 (the client app approach I demoed today) but won't hold it up for that since it's still very much experimental and there are ways around it.

/lgtm
/meow

[tekton-robot]

@AlanGreene:

In response to this:

Yeah looks good to me, tested it out locally and working nicely. I need to think a bit about how it affects #1388 (the client app approach I demoed today) but won't hold it up for that since it's still very much experimental and there are ways around it.

/lgtm
/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.