-
Notifications
You must be signed in to change notification settings - Fork 396
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
End Pr210 #221
End Pr210 #221
Conversation
… simply deprecate the overrideHookUrl, and adding additional tests
@timja ping |
src/test/java/org/jenkinsci/plugins/github/test/GitHubServerConfigMatcher.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jenkinsci/plugins/github/test/GitHubServerConfigMatcher.java
Outdated
Show resolved
Hide resolved
|
||
@Test | ||
@ConfiguredWithCode("configuration-as-code.yml") | ||
public void exportConfiguration() throws Exception { |
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.
you could probably write a simpler test with a fixture, there's nothing dynamic here so should be straightforward:
https://github.com/jenkinsci/slack-plugin/blob/master/src/test/java/jenkins/plugins/slack/jcasc/ConfigurationAsCodeTest.java#L34
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.
What is the difference with the source yaml?
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.
the source yaml is the same, the expected one doesn't have the attribute that you do a .get on, in the case of slack it's missing slackNotifier
but has all the fields under it
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 not sure also btw, final yaml is missing default return settings, is it ok?
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.
@timja i can merge&release finally this caasc support and i can accept PR for test update if you want to change it
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.
probably not worth it, it works and is unlikely to ever need to change.
Co-Authored-By: Tim Jacomb <t.jacomb@kainos.com>
Thanks @KostyaSha ! |
Thanks for CasC :) Waiting for read only global options. |
Thanks @KostyaSha for finishing this up 😊 |
This seems like it's a problem. Is there any fix looking for overriding the hook url? https://issues.jenkins-ci.org/browse/JENKINS-60738 @KostyaSha |
closes #210
This change is