feat(kubernetes): Raw resources UI MVP

[jeffherald]

This PR adds a UI module to view/manage resources returned by the raw resources controller added by @julienduchesne in spinnaker/clouddriver#5057, which can be enabled by setting the K8S_RAW_RESOURCES_ENABLED environment variable to true when running deck.

(Note: Account names and namespaces have been removed to protect the innocent)

[spinnakerbot]

The following commits need their title changed:

• 0141cb0: Raw resources view POC

• 08e0d7b: WIP commit before move

• ea1c91b: Mostly working Raw Resource Details

• be5085c: Mostly working raw resource details

• 9f486ac: Add enabling/disabling raw resources module by K8S_RAW_RESOURCES_ENABLED env var

• 68d78b2: Add css for clickable-row

• dc47351: Add kubernetes logo and minor clean-up

• ac81c7f: Remove inline styles

• 6e7f48c: Display (global) when a resource has no namespace

• 569ef1f: Fix manifest path for global resources

• 08672e5: Fix for global namespace filtering

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

[rbadillo]

@ezimanyi This is the PR for Deck for the work @julienduchesne did here:
spinnaker/clouddriver#5057
spinnaker/gate#1384

[ezimanyi]

@rbadillo : As I'm stepping away from the Spinnaker project I'll let the UI SIG and/or Kubernetes SIG take a look at this! Thanks!

[rbadillo]

Hi @german-muzquiz This is the PR for Deck for the work @julienduchesne did here:
spinnaker/clouddriver#5057
spinnaker/gate#1384

Can you help us, please?

[german-muzquiz]

Thanks for putting this!

Some usability feedback:

• The number of accounts can be very large in some installations, it could help if instead of the traditional list of account checkboxes there's an autocomplete multiselect text box, probably similar to https://react-select.com/home
• The Kind section could be grouped by category in a similar way that Lens groups kinds. This is the replacement for spinnaker's "Clusters/Load Balancers/Firewalls" categories of VM world, so kubernetes has its own categories.
• Similarly, Namespaces could use the autocomplete multiselect text box.

Question: How often is the raw resources endpoint being called, is it on demand, periodically?

I don't have much experience with react to actually merge a PR of this size, so I'll defer to someone more familiarized with the code.

[jeffherald]

Hi German,

Thanks for taking a look. While we agree that it's not from perfect in terms of UX, we viewed this as an MVP that could be a starting point for those in the community with more expertise in UX and front-end dev could iterate on in the form of enhancements after this PR has been accepted.

As far as your question, I believe the endpoint is polled every 30 seconds.

[german-muzquiz]

Makes sense, appreciate the contribution.

My only concern is in the load imposed on the server using the unfiltered endpoint, but I think polling every 30 seconds is ok.

[christopherthielen]

do you need to unusubscribe?

[christopherthielen]

Can you use the FormikFormField (preferred) or FormField (not preferred, but better than raw html) and SelectInput instead?

[christopherthielen]

Not a blocker by any means, but we are generally trying to use functional React components instead of class components going forward.

[christopherthielen]

Should this be a.localeCompare(b)?

[christopherthielen]

I recommend against mutation of the state object

[christopherthielen]

please scope all CSS using a selector with the same name as the React component it is intended for, i.e.:

.RawResource {
  .card {
    ...
  }

 .column {
   ...
 }

 ...
}

[christopherthielen]

Suggested change

	Overridable('rawResource.details');
	export class RawResourceDetails extends React.Component<IRawResourceDetailsProps, IRawResourcesDetailState> {
	@Overridable('k8s.rawResource.details')
	export class RawResourceDetails extends React.Component<IRawResourceDetailsProps, IRawResourcesDetailState> {

[christopherthielen]

If desired, type safety can be retained by writing these as:

Suggested change

	const kind = get(manifest, ['manifest', 'kind'], null);
	const kind = manifest?.manifest?.kind;

[christopherthielen]

Suggested change

	{get(manifest, ['manifest', 'status', 'conditions'], []).map((condition) => (
	{(manifest?.manifest?.status?.conditions ?? []).map((condition) => (

[christopherthielen]

you can destructure to variables with nice names

Suggested change

	{...Object.entries(groups).map((entry) => {
	return entry[0] !== 'undefined' ? (
	<RawResourceGroup title={this.groupTitle(entry[0])}>
	{...entry[1].map((resource) => <RawResource resource={resource}></RawResource>)}
	</RawResourceGroup>
	) : (
	<></>
	);
	})}
	{...Object.entries(groups).map(([key, value]) => {
	return key !== 'undefined' ? (
	<RawResourceGroup title={this.groupTitle(key)}>
	{...value.map((resource) => <RawResource resource={resource}></RawResource>)}
	</RawResourceGroup>
	) : (
	<></>
	);
	})}

[christopherthielen]

I'm pretty sure you need a key on each, i.e.: <RawResource key={something}/>

[christopherthielen]

We are trying to clean the codebase of IPromise , use PromiseLike instead

[christopherthielen]

This looks like a memory leak.

Suggested change

	this.subcribers.push({ notify: subcriber });
	const subscriber = { notify: subscriber };
	this.subcribers.push(subscriber);
	return () => this.subscribers = this.subscribers.filter(x => x !== subscriber);

[christopherthielen]

I'd probably drop this interface and put the functions directly in the subscribers array.

You can type it like so:

type ISubscriber = (message: IK8sResourcesFiltersState) => void;

Then when pushing to the list:

  public subscribe(subcriber: ISubscriber) {
    this.subcribers.push(subcriber);
    return () => this.subscribers = this.subscribers.filter(x => x !== subscriber);

[christopherthielen]

This seems redundant. What is the purpose? Also we are trying to avoid using $q where possible, especially in react components.

Suggested change

.then((kubernetesResources: IApiKubernetesResource[]) => $q.resolve(kubernetesResources));

[christopherthielen]

I requested some changes inline and offered some style advice

[jeffherald]

@christopherthielen Thank you for the feedback. I think it's all been addressed, please let me know if anything else is needed to get it merged. Cheers!

[christopherthielen]

Thank you for addressing my initial feedback. This looks much better. I've added some more suggestions which are (except for the Formik comment) mostly style or sugar suggestions. Please address at leastthe FormField feedback and then this LGTM

[christopherthielen]

...this.state, is not necessary

Suggested change

	this.setState({ ...this.state, groupBy: groupBy.toLowerCase() });
	this.setState({ groupBy: groupBy.toLowerCase() });

[christopherthielen]

Either use Formik or local state to manage state, but not both.

Suggested change

	<Formik
	onSubmit={() => null}
	initialValues={{ groupBy: 'None' }}
	render={() => {
	return (
	<FormikFormField
	onChange={this.groupByChanged}
	name="groupBy"
	label="Group By"
	input={(props) => (
	<ReactSelectInput {...props} inputClassName="groupby" stringOptions={opts} clearable={false} />
	)}
	/>
	);
	}}
	/>
	<FormField
	onChange={this.groupByChanged}
	value={this.state.groupBy}
	label="Group By"
	input={(props) => (
	<ReactSelectInput {...props} inputClassName="groupby" stringOptions={opts} clearable={false} />
	)}
	/>

[christopherthielen]

It's more idiomatic to setState with only the key(s) being changed

Suggested change

	const st = { ...this.state };
	for (const p in st.displayNamespaces) {
	if (p == RawResourceUtils.GLOBAL_LABEL) {
	st.namespaces[''] = st.displayNamespaces[p];
	}
	st.namespaces[p] = st.displayNamespaces[p];
	}
	this.setState(st);
	this.filterPubSub.publish(this.state);
	const namespaces = { ...this.state.namespaces }
	for (const p in st.displayNamespaces) {
	if (p == RawResourceUtils.GLOBAL_LABEL) {
	namespaces[''] = st.displayNamespaces[p];
	}
	namespaces[p] = st.displayNamespaces[p];
	}
	this.setState({ namespaces });
	this.filterPubSub.publish(this.state);

[christopherthielen]

I'd recommend using a different interface to type message which contains only the data that is relevant to subscribers. This avoids leaking internal component state and abstractions.

If subscribers need everything in IK8sResourcesFiltersState then disregard; this is fine.

[christopherthielen]

A little syntax sugar, in case you didn't know it:

Suggested change

	const key =
	this.props.resource.account +
	'-' +
	this.props.resource.namespace +
	'-' +
	this.props.resource.kind +
	'-' +
	this.props.resource.displayName;
	const params = {
	account: this.props.resource.account,
	name: this.props.resource.name,
	region: this.props.resource.region,
	};
	const { account, name, region, namespace, kind, displayName } = this.props.resource;
	const key = `${account}-${namespace}-${kind}-${displayName};
	const params = { account, name, region };

[christopherthielen]

this is only a curiosity: does this id get used somewhere?

[christopherthielen]

This typescript syntax sugar will retain type information. I recommend using typescript optional chaining instead of lodash .get() in most scenarios for this reason

Suggested change

	const name = get(manifest, ['manifest', 'metadata', 'name'], null);
	const kind = get(manifest, ['manifest', 'kind']);
	const name = manifest?.manifest?.metadata?.name ?? null;
	const kind = manifest?.manifest?.kind;