Refactor hacbs folder and merge into src #281

jeff-phillips-18 · 2022-12-12T13:39:10Z

Fixes

Description

Move HACBS code up into src directory. Remove hacbs specificity on local storage items. Remove hacbs css prefix.

A few components were left in the hacbs directory due to naming conflicts. These components need to get resolved to be the same component regardless of the feature flag (that is, the component should look the same), waiting on UX to determine the common look of these components.

Type of change

Code style update (formatting, renaming)
Refactoring (no functional changes, no api changes)

jeff-phillips-18 · 2022-12-12T13:39:29Z

/cc @christianvogt

codecov · 2022-12-12T13:57:57Z

Codecov Report

Merging #281 (a793686) into main (46a0be0) will increase coverage by 0.13%.
The diff coverage is 91.17%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #281      +/-   ##
==========================================
+ Coverage   73.29%   73.42%   +0.13%     
==========================================
  Files         434      436       +2     
  Lines        9163     9159       -4     
  Branches     2447     2446       -1     
==========================================
+ Hits         6716     6725       +9     
+ Misses       2309     2296      -13     
  Partials      138      138

Impacted Files	Coverage Δ
src/__data__/pipelinerun-data.ts	`100.00% <ø> (ø)`
...ApplicationDetails/ApplicationQuickstartContent.ts	`100.00% <ø> (ø)`
...ApplicationDetails/tabs/ApplicationOverviewTab.tsx	`100.00% <ø> (ø)`
...onents/ApplicationDetails/tabs/PipelineRunsTab.tsx	`80.00% <ø> (ø)`
...ew/sections/__data__/WorkflowBuildPipelinesData.ts	`100.00% <ø> (ø)`
...erview/sections/__data__/WorkflowComponentsData.ts	`100.00% <ø> (ø)`
...view/sections/__data__/WorkflowEnvironmentsData.ts	`100.00% <ø> (ø)`
...w/sections/__data__/WorkflowIntegrationTestData.ts	`100.00% <ø> (ø)`
...view/sections/__data__/WorkflowReleasePlansData.ts	`100.00% <ø> (ø)`
...overview/sections/__data__/WorkflowReleasesData.ts	`100.00% <ø> (ø)`
... and 156 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46a0be0...a793686. Read the comment docs.

rottencandy · 2022-12-15T13:20:30Z

src/components/Modals/GettingStartedModal.tsx

@@ -11,10 +11,10 @@ import {
  AlertActionLink,


We already have src/components/modal, should we move modals there instead of having two modal and Modals directories?

I was trying to leave conflicts out of this PR. There are 2 GettingStartedModal components, one for app-studio, one for hacbs. When we do the work to resolve this we can combine the directories.

rottencandy · 2022-12-15T13:20:38Z

src/components/Components/__tests__/ComponentListView.spec.tsx

-import { useGitOpsDeploymentCR } from '../../../../hooks/useGitOpsDeploymentCR';
-import { ComponentGroupVersionKind } from '../../../../models';
-import { PipelineRunGroupVersionKind } from '../../../../shared';
+import { WatchK8sResource } from '../../../dynamic-plugin-sdk';


Not important, but I think we no longer need the dynamic-plugin-sdk.ts file. All sdk types are now available in @openshift/dynamic-plugin-sdk-utils.

Trying hard not to mix in other types of rework/refactoring

rohitkrai03 · 2022-12-15T15:16:19Z

src/EnterpriseContractView/EnterpriseContractView.tsx

-  HACBS_ENTERPRISE_CONTRACT_INFO_LINK,
-  HACBS_ENTERPRISE_CONTRACT_POLICIES_DATA,
-} from './const';
+import { EnterpriseContractPolicyGroupVersionKind } from '../models';


Shouldn't these components also go inside src/components?

yes, I must have moved it incorrectly

rohitkrai03 · 2022-12-15T15:20:57Z

src/components/IntegrationTest/IntegrationTestDetailsView.tsx

@@ -13,12 +13,12 @@ import {
 } from '@patternfly/react-core';
 import { ExclamationCircleIcon } from '@patternfly/react-icons/dist/esm/icons/exclamation-circle-icon';
 import { SearchIcon } from '@patternfly/react-icons/dist/js/icons';
-import { useModalLauncher } from '../../../components/modal/ModalProvider';
-import { useNamespace } from '../../../utils/namespace-context-utils';
 import { IntegrationTestScenarioGroupVersionKind } from '../../models';


I see IntegrationTest, IntegrationTestForm and IntegrationTestListView, maybe all the integration tests related components can go into one IntegrationTest folder?

openshift-ci · 2022-12-15T17:09:14Z

@jeff-phillips-18: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

karthikjeeyar · 2022-12-16T09:53:38Z

/retest

karthikjeeyar · 2022-12-16T11:42:21Z

LGTM! Seems like the e2e failures happens in all the open PRs

rohitkrai03

@jeff-phillips-18 The hacbs=false flag doesn't work anymore for switching to old app studio UI. Apart from that everything seems to be working fine.

Edit - False alarm. It does work after a few refreshes.

sahil143 · 2022-12-16T11:51:22Z

LGTM!

rottencandy · 2022-12-16T12:07:35Z

src/components/Modals/GettingStartedModal.tsx

@@ -44,7 +44,7 @@ type Level = {
  footerButton: { title: string; cta: any };
 };

-export const LOCAL_STORAGE_KEY = 'hacbs/getting-started-modal';
+export const LOCAL_STORAGE_KEY = 'getting-started-modal';


The app studio modal is also using the same local storage key.

@rottencandy There is a ticket and a PR to remove GettingStartedModals completely. So i think we should fine with this now

Okay sounds good

This might be the reason why e2e tests are getting stuck on getting started modal.

I can put it back for now and let it be removed if/when they are completely removed.

This might be the reason why e2e tests are getting stuck on getting started modal.

No, when i checked locally cypress is able to click on the Create Application button behind the modal and proceeded with the flow. I think the issue is, Applications is not loading in the ephemeral env, you can see this issue in Abhi's PR where he removed this modal completely but still e2e is failing with that error

christianvogt · 2022-12-16T16:34:01Z

/approve

openshift-ci · 2022-12-16T16:34:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jeff-phillips-18

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:

~~OWNERS~~ [christianvogt]

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

karthikjeeyar · 2022-12-16T16:36:37Z

/lgtm

openshift-ci bot requested review from invincibleJai and jkopriva December 12, 2022 13:39

openshift-ci bot requested a review from christianvogt December 12, 2022 13:39

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2022

jeff-phillips-18 added 4 commits December 15, 2022 06:35

Update remotePlugin to not differentiate hacbs

88aa576

Remove hacbs css prefix

d0b2f59

Remove hacbs prefix from data test IDs

f3a11e2

Migrate hacbs/hooks to hooks

e7e7f5e

rottencandy reviewed Dec 15, 2022

View reviewed changes

jeff-phillips-18 force-pushed the hacbs-no-more branch from 6f98925 to cdeceba Compare December 15, 2022 14:50

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2022

rohitkrai03 requested changes Dec 15, 2022

View reviewed changes

openshift-ci bot assigned rohitkrai03 Dec 15, 2022

jeff-phillips-18 added 4 commits December 15, 2022 11:49

Migrate HACBS components and models

8fcab06

Migrate hacbs utils

b50176d

Remove hacbs specifics from integration tests

2909a70

Remove hacbs prefix from local storage items

a793686

jeff-phillips-18 force-pushed the hacbs-no-more branch from cdeceba to a793686 Compare December 15, 2022 16:49

rohitkrai03 requested changes Dec 16, 2022

View reviewed changes

rottencandy reviewed Dec 16, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2022

openshift-ci bot assigned karthikjeeyar Dec 16, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2022

rohitkrai03 merged commit aec3f1c into openshift:main Dec 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor hacbs folder and merge into src #281

Refactor hacbs folder and merge into src #281

jeff-phillips-18 commented Dec 12, 2022

jeff-phillips-18 commented Dec 12, 2022

codecov bot commented Dec 12, 2022 •

edited

rottencandy Dec 15, 2022

jeff-phillips-18 Dec 15, 2022

rottencandy Dec 15, 2022

jeff-phillips-18 Dec 15, 2022

rohitkrai03 Dec 15, 2022

jeff-phillips-18 Dec 15, 2022

rohitkrai03 Dec 15, 2022

openshift-ci bot commented Dec 15, 2022

karthikjeeyar commented Dec 16, 2022

karthikjeeyar commented Dec 16, 2022

rohitkrai03 left a comment •

edited

sahil143 commented Dec 16, 2022

rottencandy Dec 16, 2022

karthikjeeyar Dec 16, 2022

rottencandy Dec 16, 2022

rohitkrai03 Dec 16, 2022

jeff-phillips-18 Dec 16, 2022

karthikjeeyar Dec 16, 2022

christianvogt commented Dec 16, 2022

openshift-ci bot commented Dec 16, 2022

karthikjeeyar commented Dec 16, 2022

Refactor hacbs folder and merge into src #281

Refactor hacbs folder and merge into src #281

Conversation

jeff-phillips-18 commented Dec 12, 2022

Fixes

Description

Type of change

jeff-phillips-18 commented Dec 12, 2022

codecov bot commented Dec 12, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 15, 2022

karthikjeeyar commented Dec 16, 2022

karthikjeeyar commented Dec 16, 2022

rohitkrai03 left a comment • edited

Choose a reason for hiding this comment

sahil143 commented Dec 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Dec 16, 2022

openshift-ci bot commented Dec 16, 2022

karthikjeeyar commented Dec 16, 2022

codecov bot commented Dec 12, 2022 •

edited

rohitkrai03 left a comment •

edited