Skip to content
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

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 40 additions & 3 deletions src/main/java/com/rultor/agents/daemons/EndsDaemon.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand All @@ -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");
Copy link

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

boolean deprecate = false;
if(EndsDaemon.IMAGE.equals(node.xpath("//p/entry/@key").get(0))){
Copy link

Choose a reason for hiding this comment

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

@sebing we need space before {

deprecate=true;
Copy link

Choose a reason for hiding this comment

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

@sebing please provide spaces around =

}


Copy link

Choose a reason for hiding this comment

The 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;
}
Expand All @@ -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.
Copy link

Choose a reason for hiding this comment

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

@sebing dot is not needed

Copy link

Choose a reason for hiding this comment

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

@sebing moreover, we should begin with True

* @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,
Expand Down Expand Up @@ -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()
Expand All @@ -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.
Copy link

Choose a reason for hiding this comment

The 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) {
Copy link

Choose a reason for hiding this comment

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

@sebing test is missing for that code

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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? :/
Merging this would simply mean that we merge something completely untested and try it out in production, can't have that on my conscience.
http://www.yegor256.com/2009/03/04/pdd.html#best-practices explains the way this needs to go pretty well imo, make the test and add the puzzle for the production code. The other way around is not ok.

Copy link

Choose a reason for hiding this comment

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

@original-brownbear 100% agreed

directives.add("deprecate").set(
"#### Deprecation Notice ####\n" +
Copy link

Choose a reason for hiding this comment

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

@sebing we cannot let such code duplication to happen

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link

Choose a reason for hiding this comment

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

@sebing so we should use String.format and replace part that just changes

"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" +
Copy link

Choose a reason for hiding this comment

The 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");
}
}

/**
Expand Down
31 changes: 29 additions & 2 deletions src/main/java/com/rultor/agents/daemons/StartsDaemon.java
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link

Choose a reason for hiding this comment

The 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

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link

Choose a reason for hiding this comment

The 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.
Expand All @@ -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");
Copy link

Choose a reason for hiding this comment

The 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);
Expand Down Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

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

@sebing the should start with capital letter

*/
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" +
Copy link

Choose a reason for hiding this comment

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

@sebing double space here

"#####################################\n");
}

}