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

enable hacbs by default #274

Merged

Conversation

abhinandan13jan
Copy link
Contributor

@abhinandan13jan abhinandan13jan commented Dec 6, 2022

Fixes

https://issues.redhat.com/browse/HAC-2492

Description

  • set HACBS FLAG as true by default
  • HACBS can be disabled by adding to url hacbs=false
  • HACBS can be disabled by setting localstorage hacbs to true

Type of change

  • Feature
  • Bugfix
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Screen shots / Gifs for design review

Screenshot 2022-12-06 at 5 11 02 PM

How to test or reproduce?

Changes HACBS feature flag test

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

Comment on lines 4 to 11
export const IS_HACBS_DEFAULT = 'HACBS';

export const enableHACBSFlagFromQueryParam = (setFlag: SetFeatureFlag): void => {
if (IS_HACBS_DEFAULT) {
setFlag(HACBS_FLAG, true);
return;
}
let enabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

We just need to set the default value but still allow for overriding via url and local storage.

Suggested change
export const IS_HACBS_DEFAULT = 'HACBS';
export const enableHACBSFlagFromQueryParam = (setFlag: SetFeatureFlag): void => {
if (IS_HACBS_DEFAULT) {
setFlag(HACBS_FLAG, true);
return;
}
let enabled = false;
export const enableHACBSFlagFromQueryParam = (setFlag: SetFeatureFlag): void => {
let enabled = true;

Further down we must also change how we read from localStorage:

    enabled = localStorage.getItem('hacbs') === 'true';

to:

    enabled = localStorage.getItem('hacbs') !== 'false';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Merging #274 (588203e) into main (c3b321a) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #274   +/-   ##
=======================================
  Coverage   73.65%   73.65%           
=======================================
  Files         435      435           
  Lines        9182     9182           
  Branches     2455     2455           
=======================================
  Hits         6763     6763           
  Misses       2281     2281           
  Partials      138      138           
Impacted Files Coverage Δ
src/hacbs/hacbsFeatureFlag.ts 100.00% <100.00%> (ø)

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 c3b321a...588203e. Read the comment docs.

@christianvogt
Copy link
Contributor

@abhinandan13jan you'll need to update the cy tests because app-studio was the default.
Look for: localStorage.setItem('hacbs', 'true');

We'll need to set this to false for the app-studio tests for now.

@abhinandan13jan abhinandan13jan force-pushed the enable-hacbs-default branch 2 times, most recently from 2d30533 to e309b78 Compare December 8, 2022 15:04
@abhinandan13jan
Copy link
Contributor Author

@christianvogt I've updated Integration tests but they are continuously Failing with ECON_RESET

@christianvogt
Copy link
Contributor

@abhinandan13jan do they pass locally for you?

Copy link
Contributor

@christianvogt christianvogt left a comment

Choose a reason for hiding this comment

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

Need to update create-application-from-sample.spec.ts to disable the hacbs flag.

Comment on lines 39 to 40
// Need to reload the page after enabling HACBS via localStorage
cy.reload();
Copy link
Contributor

Choose a reason for hiding this comment

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

Likely don't need this anymore but do need to reload the non-hacbs tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@abhinandan13jan
Copy link
Contributor Author

the HACBS integration test runs fine for me. the hac-dev ones always times-out

Screenshot 2022-12-12 at 5 44 51 PM

@christianvogt
Copy link
Contributor

/retest

@christianvogt
Copy link
Contributor

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 12, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinandan13jan, christianvogt

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 12, 2022
@abhinandan13jan
Copy link
Contributor Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 13, 2022

@abhinandan13jan: 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.

@openshift-merge-robot openshift-merge-robot merged commit 2715a7f into openshift:main Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants