Add collection request generating function by dvail · Pull Request #3637 · stackrox/stackrox

dvail · 2022-10-31T18:14:56Z

Description

This PR adds a function to convert the FE Collection object to the expected BE request format CollectionRequest. This also cleans up some cruft in the type system and naming that surfaced during the conversion.

Type predicate functions were replaced with a discriminator field when dealing with the three resource selector types. (All resources, By name, By label). Among other improvements, this allows us to exhaustively check each condition to help ensure that when new selector types (By annotation) are added that cases are not missed.
The front end Collection type renames resourceSelectors to resourceSelector, since there is one a single one on the FE type, and renames embeddedCollections to embeddedCollectionIds since we are not dealing with full object instances.

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps
Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

Unit tests added for new generateResponse function and existing parseCollection function.

openshift-ci · 2022-10-31T18:15:00Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

dvail · 2022-10-31T18:15:08Z

Current dependencies on/for this PR:

master
- PR Implement collection attacher component. #3599
  - PR Implement read-only view of individual collection page #3636
    - PR Add collection request generating function #3637 👈
      - PR Add autocomplete for name selector dropdowns #3676
      - PR Implement create/clone/edit functionality for collection form #3644

This comment was auto-generated by Graphite.

ghost · 2022-11-02T15:45:37Z

Images are ready for the commit at 6fef19d.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.72.x-476-g6fef19da3f.

pedrottimark · 2022-11-04T18:39:38Z

ui/apps/platform/src/Containers/Collections/converter.ts

+
+    if (data.resourceSelectors.length > 1) {
+        errors.push(
+            `Multiple 'ResourceSelectors' were found for this collection. Only a single resource selector is supported in the UI. Further validation errors will only apply to the first resource selector in the response.`


Can you ask docs team to suggest a synonym for in the UI

pedrottimark · 2022-11-04T18:47:46Z

ui/apps/platform/src/Containers/Collections/converter.ts

+    return {
+        name: collection.name,
+        description: collection.description,
+        embeddedCollectionIds: collection.embeddedCollectionIds,


Can you double check it the key is embeddedCollectionIds or embeddedCollections

https://github.com/stackrox/stackrox/blob/master/proto/storage/resource_collection.proto#L28

I think the internal proto is different from what they will be sending/accepting at the service layer. I was basing the naming on the original BE design doc as well as following along with some of the PRs that get announced on the epic channel. It looks like for this type of request they are going with the ids version.

pedrottimark · 2022-11-04T18:50:03Z

ui/apps/platform/src/Containers/Collections/converter.test.ts

+                    ],
+                },
+            ],
+            embeddedCollectionIds: ['12', '13', '14'],


See comment below about this property name.

pedrottimark

Thank you for writing unit tests! A few comments.

Two observations here because independent of these changes:

Do you want to include useRestQuery as discussion item for next team meeting with goal to move from src/Containers/Dashboard/hooks to src/hooks folder?
What do you think are pro and con to distinguish which types correspond to payloads and responses versus which types are for rendering in form? That is, alternatives:
- CollectionsService.ts import SelectorField from container types file?
- first 8 lines from container types file move to CollectionsService.ts file?

dvail · 2022-11-04T20:27:00Z

1. Do you want to include `useRestQuery` as discussion item for next team meeting with goal to move from src/Containers/Dashboard/hooks to src/hooks folder?

2. What do you think are pro and con to distinguish which types correspond to payloads and responses versus which types are for rendering in form? That is, alternatives:
   
   * CollectionsService.ts import `SelectorField` from container types file?
   * first 8 lines from container types file move to CollectionsService.ts file?

Agreed, that is on my personal clean up list to move that hook up. Better yet might be to reevaluate React-Query as the standard way to handle these. I know Saif used it for the ACSCS work so maybe I can get him to throw his opinion into the mix.
This I'm not too sure on. There are some types that are clearly request/response oriented, and some that are clearly "in-container" oriented, but I'm not really sure where the best place to put the common bits is. Moving them into a top level src/types/collections directory seems neutral, but then splits types into three unrelated locations. I'd almost be inclined to weaken the types in the service file e.g. SelectorField -> string since we aren't doing runtime type validation anyway and the service layer types are more of a scout's honor that they are what they say they are. We have to run type guards on the response data in the parser anyway, so weakening the types might let us keep these decoupled without any loss of safety.

pedrottimark · 2022-11-07T14:53:18Z

Dave, what you suggest sounds good to me:

Edit CollectionsService.ts: fieldName: string consistent with https://github.com/stackrox/stackrox/blob/master/proto/storage/resource_collection.proto#L37-L46
Leave Collections/types.ts as source of truth for container

openshift-ci bot added the do-not-merge/work-in-progress label Oct 31, 2022

This was referenced Oct 31, 2022

Implement collection attacher component. #3599

Merged

Implement read-only view of individual collection page #3636

Merged

github-actions bot added the area/ui label Oct 31, 2022

dvail force-pushed the dv/collection-request-generating-function branch from 734834f to 3098e56 Compare October 31, 2022 18:20

dvail added the epic/resource-collections rox-11646 label Oct 31, 2022

dvail force-pushed the dv/ROX-12601-implement-view-mode-for-collections branch from 0905fb8 to bf65ad2 Compare November 1, 2022 14:19

dvail force-pushed the dv/collection-request-generating-function branch from 3098e56 to 3e3194e Compare November 1, 2022 14:19

dvail mentioned this pull request Nov 1, 2022

Implement create/clone/edit functionality for collection form #3644

Merged

5 tasks

dvail force-pushed the dv/ROX-12601-implement-view-mode-for-collections branch from ee2cea2 to a8ca9d0 Compare November 2, 2022 13:20

dvail force-pushed the dv/collection-request-generating-function branch from 3e3194e to fa56354 Compare November 2, 2022 13:20

dvail marked this pull request as ready for review November 2, 2022 13:48

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 2, 2022

Base automatically changed from dv/ROX-12601-implement-view-mode-for-collections to master November 2, 2022 13:51

dvail added 4 commits November 2, 2022 11:03

Add collection request generating function

b9c4dab

tests

4ab7ed6

fix test

96b4590

lint

621f668

dvail force-pushed the dv/collection-request-generating-function branch from 968b3bd to 621f668 Compare November 2, 2022 15:03

dvail requested review from bradr5 and pedrottimark November 2, 2022 15:04

dvail mentioned this pull request Nov 3, 2022

Add autocomplete for name selector dropdowns #3676

Merged

5 tasks

pedrottimark reviewed Nov 4, 2022

View reviewed changes

pedrottimark approved these changes Nov 4, 2022

View reviewed changes

Implement create/clone/edit for Collection form. (#3644)

6fef19d

dvail enabled auto-merge (squash) November 4, 2022 20:30

dvail merged commit 8288028 into master Nov 4, 2022

dvail deleted the dv/collection-request-generating-function branch November 4, 2022 21:04

ivan-degtiarenko pushed a commit that referenced this pull request Nov 5, 2022

Implement create/clone/edit for Collection form. (#3637)

06c5964

rhybrillou pushed a commit that referenced this pull request Nov 7, 2022

Implement create/clone/edit for Collection form. (#3637)

8552931

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add collection request generating function#3637

Add collection request generating function#3637
dvail merged 5 commits intomasterfrom
dv/collection-request-generating-function

dvail commented Oct 31, 2022 •

edited

Loading

Uh oh!

openshift-ci bot commented Oct 31, 2022

Uh oh!

dvail commented Oct 31, 2022 •

edited

Loading

Uh oh!

ghost commented Nov 2, 2022 •

edited by ghost

Loading

Uh oh!

pedrottimark Nov 4, 2022

Uh oh!

pedrottimark Nov 4, 2022

Uh oh!

dvail Nov 4, 2022

Uh oh!

pedrottimark Nov 4, 2022

Uh oh!

pedrottimark left a comment

Uh oh!

dvail commented Nov 4, 2022

Uh oh!

pedrottimark commented Nov 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

dvail commented Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

openshift-ci bot commented Oct 31, 2022

Uh oh!

dvail commented Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 2, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedrottimark Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

pedrottimark Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

dvail Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

pedrottimark Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

pedrottimark left a comment

Choose a reason for hiding this comment

Uh oh!

dvail commented Nov 4, 2022

Uh oh!

pedrottimark commented Nov 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dvail commented Oct 31, 2022 •

edited

Loading

dvail commented Oct 31, 2022 •

edited

Loading

ghost commented Nov 2, 2022 •

edited by ghost

Loading