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

Several fixes for operators #1602

Merged
merged 4 commits into from
Mar 26, 2020
Merged

Several fixes for operators #1602

merged 4 commits into from
Mar 26, 2020

Conversation

andresmgot
Copy link
Contributor

@andresmgot andresmgot commented Mar 24, 2020

Description of the change

I have tried with a different operator and found several issues:

  • [unrelated to operators] Some ingresses doesn't define rules, it's possible to specify just a backend ref. I have adapted the AccessURLItem to cover that case.
  • The kind property is not available when the resource is extracted from a list. Use selfLink instead.
  • When a list is empty, show a message rather than an empty list.
  • Avoid to show a message "No charts found" for the catalog view in a namespace if there are operators available.
  • Transform names into lower case before ordering items in the catalog view (if not, names starting with an upper case went first, instead that ordering things alphabetically).
  • If a ClusterServiceVersion doesn't define the resources that its CRD generates, pull all the different resources instead of rendering an empty view.
  • The status and the spec of an operator instance is missing sometimes. Don't show the related information in that case.

This an example of those fixes for the jaeger operator:

Screenshot from 2020-03-24 16-40-50

Applicable issues

@andresmgot andresmgot requested a review from absoludity March 25, 2020 11:00
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -49,6 +49,7 @@ describe("chartReducer", () => {
...initialState.selected,
error,
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, I normally see the build process (react-scripts-ts build) to catch syntactical issues like this? Ah, perhaps because that's doing a production build, it doesn't check test files? Just surprised that it got through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, I got the error trying to commit this, the pre-check that is done before commiting was failing so I guess it's an issue with react-scripts-ts.

@@ -269,7 +273,7 @@ export interface IPackageManifest extends IResource {
export interface IClusterServiceVersionCRDResource {
kind: string;
name: string;
version: string;
version?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this required for the resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually it's required but it was not being used, re-added

@andresmgot andresmgot merged commit d322fb1 into master Mar 26, 2020
@andresmgot andresmgot deleted the operatorFixes branch March 26, 2020 10:05
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.

2 participants