-
Notifications
You must be signed in to change notification settings - Fork 156
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
Fixing: Deprecate Using the yegor256/rultor Docker Image #1045
Changes from 2 commits
a755cad
bcceb7f
01077c8
962c247
7af74de
172fca3
ea9cf66
9671aab
428f49c
1cfee31
038e253
929e881
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,11 @@ public final class EndsDaemon extends AbstractAgent { | |
*/ | ||
public static final String HIGHLIGHTS_PREFIX = "RULTOR: "; | ||
|
||
/** | ||
* The default image for the project. | ||
*/ | ||
private static final String IMAGE = "yegor256/rultor"; | ||
|
||
/** | ||
* Ctor. | ||
*/ | ||
|
@@ -88,7 +93,14 @@ public Iterable<Directive> process(final XML xml) throws IOException { | |
dir, xml.xpath("/talk/@name").get(0) | ||
); | ||
} else { | ||
dirs.append(this.end(shell, dir)); | ||
final XML node = xml.node("/p/entry[@key='assets']/entry"); | ||
boolean deprecate = false; | ||
if(EndsDaemon.IMAGE.equals(node.xpath("//p/entry/@key").get(0))){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing we need space before |
||
deprecate=true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing please provide spaces around |
||
} | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing we cannot have empty lines here |
||
dirs.append(this.end(shell, dir,deprecate)); | ||
} | ||
return dirs; | ||
} | ||
|
@@ -97,11 +109,12 @@ public Iterable<Directive> process(final XML xml) throws IOException { | |
* End this daemon. | ||
* @param shell Shell | ||
* @param dir The dir | ||
* @param deprecate true if it is deprecated. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing dot is not needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing moreover, we should begin with |
||
* @return Directives | ||
* @throws IOException If fails | ||
*/ | ||
private Iterable<Directive> end(final Shell shell, | ||
final String dir) throws IOException { | ||
final String dir, final boolean deprecate) throws IOException { | ||
final int exit = EndsDaemon.exit(shell, dir); | ||
final String stdout = new ShellCommand( | ||
shell, | ||
|
@@ -135,7 +148,7 @@ public String apply(final String str) { | |
) | ||
); | ||
Logger.info(this, "daemon finished at %s, exit: %d", dir, exit); | ||
return new Directives() | ||
final Directives directives = new Directives() | ||
.xpath("/talk/daemon") | ||
.strict(1) | ||
.add("ended").set(new Time().iso()).up() | ||
|
@@ -155,6 +168,30 @@ public String apply(final String str) { | |
) | ||
) | ||
); | ||
EndsDaemon.deprecate(directives, deprecate); | ||
return directives; | ||
} | ||
|
||
/** | ||
* Adds the deprecated message to the directive. | ||
* @param directives the directives which being deprecated | ||
* @param deprecate true if it is deprecated. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing we should not have dot at the end here |
||
*/ | ||
private static void deprecate(final Directives directives, | ||
final boolean deprecate) { | ||
if (deprecate) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing test is missing for that code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mkordas I will add a puzzle for the test. Actually, I just had added the test with a puzzle and closed the issue :) Next time i will work that way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @original-brownbear are you OK with merging PR without test with just puzzle created? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mkordas @sebing no sorry, I mean you must agree that this would be as wrong as it gets right? :/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @original-brownbear 100% agreed |
||
directives.add("deprecate").set( | ||
"#### Deprecation Notice ####\n" + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing we cannot let such code duplication to happen There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mkordas I don't think, it is duplicated code. The message in the start and end are not same. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing so we should use |
||
"You are using the Rultor default Docker image " + | ||
"in your build.The Rultor has to:\n" + | ||
"1. Provide the sudo package/command and not stop " + | ||
"doing so whenever " + | ||
"a change to the Dockerfile is made, even if Rultor " + | ||
"itself does not need the sudo command.\n" + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing double space as well |
||
"2. Not install any gems to the global scope " + | ||
"that interfere with pdd or est\n" + | ||
"#####################################\n"); | ||
} | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,10 @@ | |
@ToString | ||
@EqualsAndHashCode(callSuper = false) | ||
public final class StartsDaemon extends AbstractAgent { | ||
/** | ||
* The default image for the project. | ||
*/ | ||
private static final String IMAGE = "yegor256/rultor"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing this is kind of code duplication, we need to extract this constant to separate class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mkordas I cannot find a constant class. The code is so messy to understand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing I don't meant class with constants, please read http://www.yegor256.com/2015/07/06/public-static-literals.html |
||
|
||
/** | ||
* Profile to get assets from. | ||
|
@@ -89,9 +93,14 @@ public StartsDaemon(final Profile prof) { | |
|
||
@Override | ||
public Iterable<Directive> process(final XML xml) { | ||
final Directives dirs = new Directives() | ||
final Directives directives = new Directives() | ||
.xpath("/talk/daemon[not(started)]") | ||
.strict(1) | ||
.strict(1); | ||
final XML node = xml.node("/p/entry[@key='assets']/entry"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing this line doesn't compile |
||
if(StartsDaemon.IMAGE.equals(node.xpath("//p/entry/@key").get(0))){ | ||
StartsDaemon.deprecate(directives); | ||
} | ||
final Directives dirs = directives | ||
.add("started").set(new Time().iso()).up(); | ||
try { | ||
final String dir = this.run(xml); | ||
|
@@ -248,4 +257,22 @@ private InputStream ring(final String name) throws IOException { | |
); | ||
} | ||
|
||
/** | ||
* Adds the deprecated message to the directive. | ||
* @param directives the directives which being deprecated | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing |
||
*/ | ||
private static void deprecate(final Directives directives) { | ||
directives.add("deprecate").set( | ||
"#### Deprecation Notice ####\n" + | ||
"You are using the Rultor default Docker image " + | ||
"in your build.The Rultor has to:\n" + | ||
"1. Provide the sudo package/command and not stop " + | ||
"doing so whenever " + | ||
"a change to the Dockerfile is made, even if Rultor " + | ||
"itself does not need the sudo command.\n" + | ||
"2. Not install any gems to the global scope " + | ||
"that interfere with pdd or est\n" + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebing double space here |
||
"#####################################\n"); | ||
} | ||
|
||
} |
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.
@sebing we have compilation problems here as well