-
Notifications
You must be signed in to change notification settings - Fork 169
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
fix: Propagate Premium Features flag value to configuration #19935
Conversation
@@ -156,6 +156,28 @@ public void create_propertiesAreReadFromContext() throws IOException { | |||
configuration.getStringProperty("foo", null)); | |||
} | |||
|
|||
@Test | |||
public void create_tokenFileWithPremiumFlag_premiumFlagIsPropagatedToDeploymentConfiguration() |
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.
Is this test adding anything that other tests here don't already test? This seems to just test same thing with different constant.
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.
Apparently not, because no test was failing before, though this property wasn't populated :)
Seems we don't have tests that checks that all possible properties are propagated from token file to config. I'll try to do such a test.
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.
It's a good idea to add such a test. But it will not help with new properties, unless there is for example a list of properties that has to be updated for new properties. It could be in Constants for example.
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.
This is problematic, as AbstractConfigurationFactory
uses a subset of properties that are declared in Constants
and InitParameters
, thus, we need to refactor the properties names to group them accordingly, e.g. in separate classes or by marking with comments, if it works. Looks like a bigger work, than just to write one test.
...ns/flow-dev-bundle-plugin/src/main/java/com/vaadin/flow/plugin/maven/BuildDevBundleMojo.java
Outdated
Show resolved
Hide resolved
…low/plugin/maven/BuildDevBundleMojo.java Co-authored-by: Tomi Virtanen <tltv@vaadin.com>
Quality Gate passedIssues Measures |
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.
Approving. Test can be updated also later.
Description
This adds missing codes into app configuration factory that propagates the PREMIUM_FEATURES flag value from token file to app config. This value can be retrieved from deployment configuration in runtime.
Related-to #19846
Type of change
Checklist
Additional for
Feature
type of change