Skip to content
This repository was archived by the owner on Jan 23, 2020. It is now read-only.

Creating abstract class, returning booleans, removing dead code, etc..#161

Merged
gzussa merged 11 commits into
masterfrom
gzu/clean_up
Oct 1, 2019
Merged

Creating abstract class, returning booleans, removing dead code, etc..#161
gzussa merged 11 commits into
masterfrom
gzu/clean_up

Conversation

@gzussa

@gzussa gzussa commented Aug 16, 2019

Copy link
Copy Markdown
Contributor

What this PR does.

  • Grouped all event classes inside their own package events. They all extends an abstract class called AbstractDatadogEvent with common methods to all event implementations. I all added an common interface called DatadogClient.
  • I also created a model called BuildData to collect build informations. Before we would use a JSONObject that was harder to work with.
  • I changed the DatadogHttpRequest class into the DatadogHttpClient. This class cannot be instantiated and can now be leased from the descriptor. The same way we used to do with the StatdDClient. DatadogHttpClient implement a new interface called DatadogClient. This interface is used to create stubs for testings. See more details below.
  • DatadogHttpClient now only has methods related to submitting data to datadog. As a consequence, I was able to remove "duplicated-ish" submission methods (eg: the gauge or the serviceCheck method) that were part of other classes like the DatadogBuildListener. I was also able to group all the submission logic inside this new client (that includes constants variables as well)
  • Code for the onStarted, the onCompleted, the onCheckout and the doRun method is now simpler and more readable.
  • This code refactoring also simplified the content of the DatadogUtilities class. I moved methods to better locations if needed. When used only once, I often removed the method. When called only within the same class, I changed their scope to private.
  • I added test for all events classes, I created a DatadogClientStub and a DatadogStatsDClientStub classes to extend testing capabilities. As a consequence I created more tests to the DatadogBuildListener (onStarted method in particular)
  • General linting

What is next

  • Add more tests in general over time.

@nmuesch nmuesch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey, Overall looks good, solid refactor work 💯🏅💜 Took a first pass review and left some comments/questions.

Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/DatadogJobProperty.java Outdated
Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/clients/DatadogHttpClient.java Outdated
Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/clients/DatadogHttpClient.java Outdated
Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/clients/DatadogHttpClient.java Outdated
@gzussa

gzussa commented Sep 30, 2019

Copy link
Copy Markdown
Contributor Author

I manually tested:

  • adding tags using a property file in the job workspace.
  • adding tags using list at the job level.
  • setting a hostname in the global plugin config.
  • blacklisted a job in the global plugin config.
  • whitelisting a job in the global plugin config.
  • adding global job tags in the global plugin config.
  • adding Optional Tags (node) in the global plugin config.
  • the integration with the dogStatsD server.

@nmuesch nmuesch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left comments on the Testing.md file as we discussed 😄Thanks for this.

Comment thread TESTING.md Outdated
Comment thread TESTING.md Outdated
Comment thread TESTING.md Outdated
Comment thread TESTING.md Outdated
Comment thread TESTING.md Outdated
Comment thread TESTING.md Outdated
Comment thread TESTING.md Outdated
Comment thread TESTING.md Outdated
Comment thread TESTING.md Outdated
Comment thread TESTING.md Outdated
Comment thread TESTING.md Outdated
Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/DatadogBuildListener.java Outdated
Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/DatadogBuildListener.java Outdated
Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/DatadogUtilities.java Outdated
Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/DatadogUtilities.java Outdated

@nmuesch nmuesch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one last thing in the testing file :)

Comment thread TESTING.md Outdated
- You can add `node` optional tag by clicking on the node checkbox
- Click on the "Test Key" to make sure your key is valid.
- You can set your machine `hostname`.
- `.*, owner:$1, release_env:$2, optional:Tag3`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this for the fields textbox? Can we clarify that at the beginning of this line?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oups Good catch!

@nmuesch nmuesch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@gzussa
gzussa merged commit f51666d into master Oct 1, 2019
@gzussa
gzussa deleted the gzu/clean_up branch October 1, 2019 09:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants