Skip to content

Refactor: Introduce declarative configuration layer#86

Merged
Aki-7 merged 4 commits into
jenkinsci:mainfrom
Aki-7:refactor-configuration
Aug 17, 2021
Merged

Refactor: Introduce declarative configuration layer#86
Aki-7 merged 4 commits into
jenkinsci:mainfrom
Aki-7:refactor-configuration

Conversation

@Aki-7

@Aki-7 Aki-7 commented Aug 12, 2021

Copy link
Copy Markdown
Member

Preparation for #71 #39 .

Enable to load configurations like this.

@Configuration
public class Config {
    @ConfigOption(env = "OTEL_EXPORTER_OTLP_ENDPOINT", required = true)
    public String endpoint;

    @ConfigOption(env = "SOME_ENV")
    public Pattern value = Pattern.compile("^default value$");
}
Config conf = new Config();
ConfigParser parser = new ConfigParser(conf);
parser.parse();

It supports loading config from environment variables using a env attribute.
And I'm planning to add more attributes if needed to load configurations from files, system properties, etc...

Checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • All follow-ups are documented as issues and-or TODO comments
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue
  • Please update user documentation if needed
  • Please update developer documentation if needed

@Aki-7 Aki-7 requested a review from a team as a code owner August 12, 2021 06:17
@Aki-7 Aki-7 requested review from stellargo and removed request for a team August 12, 2021 06:17
@oleg-nenashev oleg-nenashev self-requested a review August 12, 2021 14:12

@stellargo stellargo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks great overall :) Still trying out the plugin to leave any comments from functional perspective, but leaving few minor comments

public String serviceInstanceId = UUID.randomUUID().toString();

@ConfigOption(env = "REMOTING_OTEL_METRIC_FILTER")
public Pattern metricsFilterPattern = Pattern.compile(".*");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

hey, you can define ".*" as a common constant

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is possible, but overall it is an overkill when there is just one usage

try {
configParser.parse();
} catch (ConfigurationParseException e) {
LOGGER.log(Level.WARNING, "Failed to road configuration", e);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

similarly, can define this string in common constants as well

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same. OTOH there is a typo

Suggested change
LOGGER.log(Level.WARNING, "Failed to road configuration", e);
LOGGER.log(Level.WARNING, "Failed to load configuration", e);

}

@Test
public void testInvalidConfigOption() throws Exception {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

instead of asserting for boolean hasError you can do:
@Test(expected = ConfigurationParseException.class)

import java.util.regex.Pattern;

public class PatternOptionHandlerTest {
static final String TEST_ENV1= "REMOTING_OTEL_TEST_ENV1";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

linting

@oleg-nenashev oleg-nenashev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is a typo that needs to be fixed. Also I would suggest adding documentation in the same pull request.

P.S: This framework might also be an overkill and may create issues if we ever try to package Jenkins agents as native images. It is not a problem in foreseeable future

doParse();
} catch (ConfigurationParseException e) {
throw e;
} catch (Throwable e) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is not the best practice in general, because it would catch 'Error's that should be fatal. In this case it leads to a failure

@Aki-7 Aki-7 requested a review from oleg-nenashev August 16, 2021 07:37
@Aki-7 Aki-7 merged commit 2fabd14 into jenkinsci:main Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants