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

RHCLOUD-33912: update crd spec to match chrome-service #193

Conversation

florkbr
Copy link
Contributor

@florkbr florkbr commented Oct 7, 2024

This PR aligns the FEO frontend modules with the current structure in chrome service backend. This sync is to allow for additional updates to the FEO in the future.

@florkbr florkbr marked this pull request as ready for review October 14, 2024 12:55
@florkbr
Copy link
Contributor Author

florkbr commented Oct 14, 2024

Did not mean to convert this to a PR from draft yet. Looking into the build pipeline failure (running make build-template locally)

ModuleConfig *ModuleConfig `json:"moduleConfig,omitempty" yaml:"moduleConfig,omitempty"`
FullProfile *bool `json:"fullProfile,omitempty" yaml:"fullProfile,omitempty"`
DefaultDocumentTitle string `json:"defaultDocumentTitle,omitempty" yaml:"defaultDocumentTitle,omitempty"`
IsFedramp *bool `json:"isFedramp,omitempty" yaml:"isFedramp,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

A note for the fedramp. We should be thinking about sunsetting this attribute as it will be clear form the environment which modules are deployed and by extension eligible for the env.

}

type Module struct {
ID string `json:"id" yaml:"id"`
Module string `json:"module" yaml:"module"`
Routes []Route `json:"routes" yaml:"routes"`
Dependencies []string `json:"dependencies,omitempty" yaml:"dependencies,omitempty"`
OptionalDependencies []string `json:"optionalDependencies,omitempty" yaml:"optionalDependencies,omitempty"`
Dependencies []string `json:"dependencies,omitempty" yaml:"dependencies,omitempty"` // not in the current chrome-service spec
Copy link
Contributor

Choose a reason for hiding this comment

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

These are not used. We should investigate and if no modules are suing these, we should remove them.

Dynamic bool `json:"dynamic,omitempty" yaml:"dynamic,omitempty"`
Exact bool `json:"exact,omitempty" yaml:"exact,omitempty"`
Props *apiextensions.JSON `json:"props,omitempty" yaml:"props,omitempty"`
FullProfile bool `json:"fullProfile,omitempty" yaml:"fullProfile,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

We should no longer use the full profile. Attribute. A candidate for an Action required email. After that we can remove it.

Exact bool `json:"exact,omitempty" yaml:"exact,omitempty"`
Props *apiextensions.JSON `json:"props,omitempty" yaml:"props,omitempty"`
FullProfile bool `json:"fullProfile,omitempty" yaml:"fullProfile,omitempty"`
IsFedramp bool `json:"isFedramp,omitempty" yaml:"isFedramp,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

^^

type Permission struct {
Method string `json:"method" yaml:"method"`
Apps []string `json:"apps,omitempty" yaml:"apps,omitempty"`
Args []string `json:"args,omitempty" yaml:"args,omitempty"` // TODO validate array item type (string?)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be array with mixed types. [string, number, etc]. Can we use the apiextensions.JSON here? In my head [] is a valid JSON and we might be able to get away with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Hyperkid123 done! Verified locally by updating the kuttl basic frontend test with:

module:
    config:
      ssoUrl: 'https://'
    manifestLocation: /apps/chrome/js/fed-mods.json
    modules:
        - id: "landing"
          module: "./RootApp"
          routes:
          - pathname: /
            exact: true
            permissions:
                - method: withEmail
                  args:
                    - "@redhat.com"
                    - 2

Now passes with the JSON type:

=== CONT  kuttl/harness/basic-frontend
    logger.go:42: 10:40:24 | basic-frontend | Creating namespace: kuttl-test-evident-leech
    logger.go:42: 10:40:24 | basic-frontend/0-create-namespace | starting test step 0-create-namespace
    logger.go:42: 10:40:24 | basic-frontend/0-create-namespace | Namespace:/test-basic-app created
    logger.go:42: 10:40:24 | basic-frontend/0-create-namespace | test step completed 0-create-namespace
    logger.go:42: 10:40:24 | basic-frontend/1-create-resources | starting test step 1-create-resources
    logger.go:42: 10:40:24 | basic-frontend/1-create-resources | FrontendEnvironment:/test-basic-app-environment created
    logger.go:42: 10:40:24 | basic-frontend/1-create-resources | Frontend:test-basic-app/chrome created
    logger.go:42: 10:40:24 | basic-frontend/1-create-resources | test step completed 1-create-resources
    logger.go:42: 10:40:24 | basic-frontend/2- | starting test step 2-

@Hyperkid123 Hyperkid123 merged commit d613e23 into RedHatInsights:main Oct 14, 2024
11 checks passed
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.

3 participants