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

Update user settings hook to use bridge call to create ConfigMap #7327

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

jerolimov
Copy link
Member

@jerolimov jerolimov commented Nov 25, 2020

Fixes:
https://issues.redhat.com/browse/ODC-4755 https://issues.redhat.com/browse/ODC-5189

This PR is based on #7095 which must be merged first (the go part).

It keep this to make it also easier to test the frontend change. A review here should focus on the TS change.

Analysis / Root cause:
Based on the User Settings for OpenShift Console enhancement proposal we want to create a ConfigMap for each individual user in the namespace openshift-console-user-settings

Currently you need create this namespace yourself, see Test Setup below. This namespace will be automatically created by the console-operator, see PR openshift/console-operator#479

Solution Description:
Because the user could not create a ConfigMap here, we create a new ConfigMap, Role and RoleBinding with the service account role over the bridge. This code provides 3 new endpoints to create, get and delete the user settings. (The second and third API calls primary for debugging / development.) The console will fetch the ConfigMap over the known k8sWatch API.

Screen shots / Gifs for design review:
Irrelevant

Unit test coverage report:
Updated user-settings.spec.ts and useUserSettings.spec.ts:

 PASS  packages/console-shared/src/utils/__tests__/user-settings.spec.ts
  createConfigMap
    ✓ calls user settings api  (4ms)
  updateConfigMap
    ✓ calls user settings api  (1ms)
  deseralizeData
    ✓ does not convert null or undefined
    ✓ converts valid json to an object (1ms)
    ✓ return invalid json strings as string
  seralizeData
    ✓ does not convert strings
    ✓ converts objects

 PASS  packages/console-shared/src/hooks/__tests__/useUserSettings.spec.ts
  useUserSettings
    ✓ should create and update user settings if watcher could not find a ConfigMap (297ms)
    ✓ should return default value for an empty configmap after switching from loading to loaded (4ms)
    ✓ should return saved value for an known key after switching from loading to loaded (3ms)
    ✓ should return default value for an unknown key if data is already loaded (hook is used twice) (3ms)
    ✓ should return saved value for an known key if data is already loaded (hook is used twice) (2ms)
    ✓ should return latest user settings value after switching from loading to loaded (3ms)
    ✓ should return latest user settings value in loaded state (hook is used twice) (2ms)
    ✓ should trigger update user settings when setter was called (3ms)
    ✓ should fallback to localStorage if creation fails with 403 (must not call updateConfigMap) (4ms)

Test Suites: 2 passed, 2 total
Tests:       16 passed, 16 total

Test setup:
This PR require some additional RBAC rules.

  1. If PR Add namespace and role resources for user settings console-operator#479 is not part of your cluster you need to create a namespace:

1a Create namespace:

apiVersion: v1
kind: Namespace
metadata:
  name: openshift-console-user-settings
  annotations:
    include.release.openshift.io/self-managed-high-availability: "true"
    openshift.io/node-selector: ""

Until https://github.com/openshift/console-operator/pull/486/files is merged and available on your cluster you need to add this as well:

1b Create Role:

kind: Role
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: console-user-settings-admin-fixed
  namespace: openshift-console-user-settings
  annotations:
    include.release.openshift.io/self-managed-high-availability: "true"
rules:
- apiGroups:
  - rbac.authorization.k8s.io
  resources:
  - roles
  - rolebindings
  verbs:
  - get
  - list
  - watch
  - create
  - update
  - delete
  - patch
- apiGroups:
  - ""
  resources:
  - configmaps
  verbs:
  - get
  - list
  - watch
  - create
  - update
  - delete
  - patch

1c Create RoleBinding:

kind: RoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: console-user-settings-admin-fixed
  namespace: openshift-console-user-settings
  annotations:
    include.release.openshift.io/self-managed-high-availability: "true"
roleRef:
  kind: Role
  name: console-user-settings-admin-fixed
  apiGroup: rbac.authorization.k8s.io
subjects:
  - kind: ServiceAccount
    name: console
    namespace: openshift-console
  1. You need the backend functions from Add API to create user settings #7095.

You can checkout 7095, build the backend with ./build-backend.sh and switch back to this PR.

  1. You need a hook to test PR. I used this AddPage.tsx:
import * as React from 'react';
import { Helmet } from 'react-helmet';
import { useTranslation } from 'react-i18next';
import { match as RMatch } from 'react-router';
import { useUserSettings, useUserSettingsCompatibility } from '@console/shared';

import { Firehose } from '@console/internal/components/utils';
import ODCEmptyState from './EmptyState';
import NamespacedPage from './NamespacedPage';
import ProjectsExistWrapper from './ProjectsExistWrapper';
import CreateProjectListPage from './projects/CreateProjectListPage';

export interface AddPageProps {
  match: RMatch<{
    ns?: string;
  }>;
}

interface AddUserSettings {
  value: string;
}

const AddPage: React.FC<AddPageProps> = ({ match }) => {
  const { t } = useTranslation();
  const namespace = match.params.ns;

  const [userSettings, setUserSettings] = useUserSettings<AddUserSettings>('a_user_settings');
  const [userSettingsComp, setUserSettingsComp] = useUserSettingsCompatibility<string>(
    'a_user_settings_comp',
    'lskey',
  );

  return (
    <>
      <Helmet>
        <title>{`+${t('devconsole~Add')}`}</title>
      </Helmet>
      <NamespacedPage>
        <h1>
          user settings:
          <pre>{JSON.stringify(userSettings)}</pre>
          <button type="button" onClick={() => setUserSettings({ value: 'AAAAA' })}>
            AAAAA
          </button>
          <button type="button" onClick={() => setUserSettings({ value: 'BBBBB' })}>
            BBBBB
          </button>
        </h1>
        <h1>
          user settings compatibility:
          <pre>{JSON.stringify(userSettingsComp)}</pre>
          <button type="button" onClick={() => setUserSettingsComp('AAAAA')}>
            AAAAA
          </button>
          <button type="button" onClick={() => setUserSettingsComp('BBBBB')}>
            BBBBB
          </button>
        </h1>
        <Firehose resources={[{ kind: 'Project', prop: 'projects', isList: true }]}>
          <ProjectsExistWrapper title={t('devconsole~Add')}>
            {namespace ? (
              <ODCEmptyState title={t('devconsole~Add')} />
            ) : (
              <CreateProjectListPage title={t('devconsole~Add')}>
                {t('devconsole~Select a project to start adding to it')}
              </CreateProjectListPage>
            )}
          </ProjectsExistWrapper>
        </Firehose>
      </NamespacedPage>
    </>
  );
};

export default AddPage;

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci-robot openshift-ci-robot added the component/backend Related to backend label Nov 25, 2020
@openshift-ci-robot openshift-ci-robot added the component/shared Related to console-shared label Nov 25, 2020
@jerolimov
Copy link
Member Author

/unassign @rhamilto @jhadvig
/assign @christianvogt @invincibleJai
/hold
Because it depends on #7095

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 25, 2020
@jerolimov
Copy link
Member Author

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2020
@jerolimov
Copy link
Member Author

/retest

@jerolimov jerolimov force-pushed the odc-4755-hook branch 2 times, most recently from a1115d4 to 957112e Compare November 27, 2020 06:48
};
// Use JSON Merge Patch instead of normal JSON Patch because JSON Patch calls
// fail if there is no data defined.
const response = await coFetchJSON(url, 'PATCH', {
Copy link
Member

@invincibleJai invincibleJai Nov 27, 2020

Choose a reason for hiding this comment

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

will it make sense to put this in try catch and return response or throw err there

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately coFetchJSON doesn't return a response, it returns already the parsed content. I can switch to coFetch and handle status code checking and response.json() myself. I will try that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to coFetch, please take a look

@jerolimov jerolimov force-pushed the odc-4755-hook branch 2 times, most recently from bcbd77a to b06c1a7 Compare November 27, 2020 10:07
@jerolimov
Copy link
Member Author

I'm not happy with the unit tests yet. I currently rework them, I don't expect a change in the hook.

@christianvogt
Copy link
Contributor

▶ oc whoami
kube:admin

▶ ./contrib/oc-get-user-settings.sh
Get ConfigMap "user-settings-<no value>" from namespace "openshift-console-user-settings" and save it to "user-settings.yaml"

Error from server (NotFound): configmaps "user-settings-<no value>" not found

@christianvogt
Copy link
Contributor

Settings saved and loaded as expected for kubeadmin user in local dev.

@jerolimov
Copy link
Member Author

jerolimov commented Nov 30, 2020

@invincibleJai Can you please double check the unit tests, hope you are fine with them.

@christianvogt Fixed the get/set user settings script as well. Works now fine with other users. Also added the option to fetch other user settings from other usernames.

➜  contrib/oc-get-user-settings.sh 
Get ConfigMap "user-settings-kubeadmin" from namespace "openshift-console-user-settings" and save it to "user-settings.yaml"
  ...

➜  contrib/oc-get-user-settings.sh developer
Get ConfigMap "user-settings-12341234-1234-1234-1234-123412341234" from namespace "openshift-console-user-settings" and save it to "user-settings.yaml"
  ...

edit: Added also an example to git and a second option to define the filename. What do you think?

@jerolimov
Copy link
Member Author

/retest

@jerolimov
Copy link
Member Author

jerolimov commented Dec 1, 2020

@invincibleJai Updated all tests, I'm happy now, wdyt? Did you see a missing case? Tests also require some minimal tweaks in the hook.

@jerolimov
Copy link
Member Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 1, 2020
@invincibleJai
Copy link
Member

@invincibleJai Updated all tests, I'm happy now, wdyt? Did you see a missing case? Tests also require some minimal tweaks in the hook.

Thanks @jerolimov this looks good to me , looks like we covered all.

@invincibleJai
Copy link
Member

/lgtm

Verified the changes, works as expected

@invincibleJai
Copy link
Member

/test e2e-gcp-console

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020
Comment on lines 1 to 5
apiVersion: v1
kind: ConfigMap
data:
console.communityProvidersWarning: "true"
console.guidedTour: '{"dev":{"completed":true}}'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this sample?
What's the intended use?
Might be better to simply have a comment in the useUserSettings hook.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the example

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020
@jerolimov
Copy link
Member Author

Just removed the example

@christianvogt
Copy link
Contributor

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020
@jerolimov
Copy link
Member Author

/retest

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2020
@christianvogt
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, invincibleJai, jerolimov

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:

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

@jerolimov
Copy link
Member Author

And another rebase was required after #7051 was merged. Will create a new PR for the scripts.

@openshift-merge-robot openshift-merge-robot merged commit 821797c into openshift:master Dec 2, 2020
@spadgett spadgett added this to the v4.7 milestone Dec 9, 2020
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. component/backend Related to backend component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants