-
Notifications
You must be signed in to change notification settings - Fork 361
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
upcoming: [M3-7670] - Add and handle ACLB Account Capability #10098
upcoming: [M3-7670] - Add and handle ACLB Account Capability #10098
Conversation
environmentVar: boolean, | ||
capabilities: AccountCapability[] | ||
) => { | ||
return environmentVar || capabilities.indexOf(featureName) > -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capabilities.indexOf(featureName) > -1
is savage. I wonder if includes
did not exist when this was written 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I de-ramda-ifyed this and tried to make the comment more useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd been confused by isFeatureEnabled
before too. My past, present, and future self thanks you for documenting this function for better clarity. 🙌🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I de-ramda-ifyed this function, this test needed updating.
I tried to clean up this test and make it more straightforward. Previously it was saying all kinds of irrelevant stuff like EAP
and beta_programs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd been confused by isFeatureEnabled
before too. My past, present, and future self thanks you for documenting this function for better clarity. 🙌🏼
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Coverage Report: ✅ |
); | ||
|
||
return { isACLBEnabled }; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, should be make this a pattern to follow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! It's a nice simple abstraction. I did it just incase we need to change how isACLBEnabled
is determined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description 📝
This PR adds
Akamai Cloud Load Balancer
to account capabilities so that we can conditionally enable the feature for some users during the closed beta.Changes 🔄
Akamai Cloud Load Balancer
to account capabilitiesHow to test 🧪
Prerequisites
aclb
tag to your account in the dev environment (ask me in Slack for more detailed instructions on how to do this)Verification steps
In summary, ACLB should only be visible if the
aglb
feature flag is on or you have theaclb
tag on your accountAs an Author I have considered 🤔