Sunday, May 21, 2017

Good Reasons to Dockerize Builds

The Problem

On my current team, we use a common tool stack of Jenkins, Maven, Robot Framework, and Java. Our build system is centralized with a couple of dozen projects. We have suffered build and developer friction, namely
  • Our version of Jenkins is older than the Internet and cannot be easily upgraded
  • Upgrading a plugin/tool risks incompatibility with other plugins/tools
  • We have accumulated snippets of shell scripts embedded int POM files
  • Developers sometimes use our continuous integration (CI) to do compiles and test code
  • We sometimes have trouble with local builds passing and CI builds failing
These and more are the Usual Suspects on almost any build system that has grown over time. Having been on this team for around two years now, I have identified a direction we should take to modernize our build system.

A Solution


Docker and containerized builds have been around since at least 2013. There are some compelling reasons to consider using containerized builds. This is my attempt to clearly enumerate the advantages and to convince my chain-of-command to make the jump. Hopefully this will help any readers to do the same.

Moving to containers means we can deploy onto a cloud with minimal work. This can address scaling issues effectively. Note that some builds will still depend on lab access to local devices, and these dependencies will not scale.

Containerizing the build pipeline means easier upgrading. For example, running a component in its own container isolates it so other containers that depend on it are forced through a standard, explicit interface.

Containerizing the build means better partitioning. So instead of making environments that contain all possible tools and libraries, a container can only use those needed for its specific purpose. This has the side effect of reducing defects due to interacting 3rd party components.

Containerizing the build means a clean reset state. Instead of writing reset scripts, the container is discarded and resurrected from a fixed image. This is a phoenix (burn-it-down) immutable view of containers, and forces all build and configuration to be explicit (not accumulate in snow flake instances).

Containerizing means 100% consistency between local development and the build system, which should eliminate the but-it-works-on-my-machine problem.

Containerizing the build means effective postmortems for build failures, potentially leaving failed runs in the exact state of failure, rather than relying solely on extracted log files.

Containerizing the build means building and installing an intermediate, shared artifact onces, instead of 52 times, and potentially speeds up the build cycle.

Containerizing the build means that some tests can make temporary checkpoints via a temporary image/container and roll back to that state rather than tearing  down and rebuilding, affording a quicker build.

Judicious use of containers might help with diagnosing hard-to-reproduce issues in the field. I have seen instance of technical support sending/receiving VM images to/from customers. Containers would be both simpler and could be a lot smaller.

Containerizing the build is considered a modern best-practice and affords access to all kinds of build work flow and management tools.

That's it! Good luck convincing your management when the time comes to modernize your build systems.

Monday, April 3, 2017

On CheckStyle and Coding Standards

Recently I was given the opportunity to work on an open source project. My team had developed a new additional feature and we were eager to get is pushed upstream before a code freeze milestone. Our team was closed source and project was proprietary, so this effort was essentially to push a portion upstream as open source.

We decided to do three pushes due to various time and resource constraints: push the basic feature, then an enhancement, then some automated integration tests. Our team was not familiar with the process or requirements of the upstream project but two thirds of the code was written under contract to us by a former major upstream committer to that project.

The push of the basic feature was rougher than I thought, given the authorship of much of the code. Most of the issues were stylistic and our reliance on the contractor to "do the right thing" had been a bit shakey. After some corrective actions, the first merge happened. Based on code review issues raised on the first push, I revised the second batch of code, and submitted a pull request. As expected, there were a few minor issues raised which I corrected.

So this project has checkstyle enabled as warnings. But the build is so full of noise and warnings (that is deserving of another rant on its own) that developers tend to ignore warnings, especially those involving things like whitespace, which happens a lot with checkstyle.

So I submitted the pull request and sort of expected an approval and merge. This was highly desirable because the code freeze deadline was only a couple of days away. Instead what I got was a request to move the checkstyle setting from warning level (the project default) to error level.

This was a stunning request for several reasons.

* First, making the code freeze deadline was now unlikely.

* Second, this would be a new requirement on our code that was not met by any of the hundreds of thousands of lines of code that already existed. Plus our previous commit was accepted without that obligation.

* Third, the time to enable something like checkstyle is day zero of a project. Turn it on then and let issues be fixed as part of every single commit. If you have never turned on an analyzer like checkstyle on to an unsuspecting code base, it is dismal. Our commit was probably a few thousand lines of code, but checkstyle emitted about 250 "errors". This would be such gems as "there is a space between your method name and the parameter list". Manually fixing all of these would be tedious and expensive - probably several days of work.

There were a half-dozen "functional" issues that I fixed, like "unused import" and "catching naked exceptions" that were entirely reasonable to ask for a fix. Those functional fixes were done and the new pull request has been awaiting approval for three days now. I guess in open source, not everyone has a sense of urgency. Complicating this, your reviewer might actually work for a competing company, so politics might interlope.

Some observations...

I wanted to point out an issue of open versus closed source. In the past, on smaller closed source teams, we never enforced cosmetic coding standards. Everyone's slightly different coding styles acted as fingerprints so you could tell without looking who the developer was. But in an open source context, where there could be large numbers of developers, it makes a lot of sense to have strict coding standards and enforecment of those standards should be enabled by default.

Readability is subjective. The wars of bracket placement, and other whitespace issues, happen all the time and clearly have no real impact to the business. To me, a perfect solution would be that whitespace is always managed by the tools locally to each developer. For example, a developer's whitespace style is applied to the code base upon "get", does not show up as a "diff", and is removed as part of a "push".

The imperfect solution is the best we have today. You can get plug-ins into IntelliJ or Eclipse that will show you violations in "real time" so that you can take instant corrective actions. Unfortunately there is no comprehensive code reformatting tool to manage style violations. Many of them are amenable to automated fixes, but not all of them.

We have some ways to go before we can have our cake and eat it too. Maybe it will happen when we have spent enough expensive developer time chasing down that stray space in our code-under-review.

Monday, March 27, 2017

A New Gerrit User

Recently I had to commit work upstream to the public Open Daylight project. As part of that effort, I had to learn some new tools, particularly Gerrit. This is a code review/management tool that works with Git, which I was already familiar with.

Here are some ad hoc things I learned as part of committing work upstream...

Gerrit has a different workflow that you are likely using in downstream coding with other tools. There is a general tutorial and an ODL coding introduction to working with Gerrit.

We followed our own coding guidelines downstream and those did not match to the Google coding conventions used upstream. Differences we ran into were:

• Lower-case method names, even for JUnit test methods (I won't push back with Spock conventions) • Using the most restrictive visibility for access qualifiers (I won't push back the philosophy of library design)
• Use Java 8 lambdas everywhere possible

Gerrit (plugin) has some non-obvious controls, namely that you can kick off another build by putting "recheck" as your comment. Others are "rerun", "verify" as in here.

Upstream coders usually add the prefix "WIP: " to their bug report to let other developers know things are not ready for prime time reviews yet.

Mechanically you can review the issues by using "Diff against:" drop list to pick a revision to start at then go into the first file and then use the green upper right arrows to navigate between files. (It'd be nice if Gerrit had a "next feedback item" navigation).

The master branch on ODL can and does break, causing lots of down time with broken builds (days even).

Full builds take 10 hours and verifying-before-pushing is an evolving process.

If you are using "git review" and you forgot the "--amend" option on your commit, you will be prompted to squash and will end up with a new branch. You can recover using "git rebase -i HEAD~2" then using the force option and then abandoning the new branch.

Along with master breaking, master can also produce stale artifacts, so don't assume that clearing your m2 cache, creating a subproject repo and building will give you up-to-date code.

You can search jar files using find . -type f -name '*.jar' -print0 | xargs -0 -I '{}' sh -c 'jar tf {} | grep "YourClassHere" && echo {}' for helping verify your m2 has the latest.

The patch workflow is not very good at all for having multiple people working on an item; traditional git branches and management are superior in this respect so expect some pain.

If your patch won't build because master is broken, you can rebase it to pick up a fix temporarily. When you do a "git review" to push it, you will be prompted because there are two commits (your new patch revision and the fix). You do not want to merge that fix into your patch.

Hopefully, some of these tips will find their way to helping someone else along the way.

Saturday, November 19, 2016

The Cost of Separation of Concerns

One of the things I enjoy doing is looking at other developers' code and dissecting it. You can find a lot of advice on how to refactor code (including this blog). One that caught my eye recently was this one. It shows a small C# example and how to refactor/improve it.

Go ahead and read it. It'll take 5 minutes.

I'll wait.

So I agree that those changes were probably improvements to the code. But when I see code, I see layers and coupling and dependencies. So I thought I'd post a refactoring here that is very different than you'll see in most places.

First, the original code...


 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
 public class Invoice  
 {  
   public long Amount { get; set; }  
   public DateTime InvoiceDate { get; set; }  
   public void Add()  
   {  
     try  
     {  
       // Code for adding invoice  
       // Once Invoice has been added , send mail   
       MailMessage mailMessage = new MailMessage  
 ("MailAddressFrom","MailAddressTo","MailSubject","MailBody");  
       this.SendEmail(mailMessage);  
     }  
     catch (Exception ex)  
     {  
        System.IO.File.WriteAllText(@"c:\Error.txt", ex.ToString());  
     }  
   }  
   public void Delete()  
   {  
     try  
     {  
       // Code for Delete invoice  
     }  
     catch (Exception ex)  
     {  
       System.IO.File.WriteAllText(@"c:\Error.txt", ex.ToString());  
     }  
   }  
   public void SendEmail(MailMessage mailMessage)  
   {  
     try  
     {  
       // Code for getting Email setting and send invoice mail  
     }  
     catch (Exception ex)  
     {  
       System.IO.File.WriteAllText(@"c:\Error.txt", ex.ToString());  
     }  
   }  
 }  

And now the original refactored code...


 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
 public class Invoice  
 {  
   public long Amount { get; set; }  
   public DateTime InvoiceDate { get; set; }  
   private FileLogger fileLogger;  
   private MailSender mailSender;  
   public Invoice()  
   {  
     fileLogger = new FileLogger();  
     mailSender = new MailSender();  
   }  
   public void Add()  
   {  
     try  
     {  
       fileLogger.Info("Add method Start");  
       // Code for adding invoice  
       // Once Invoice has been added , send mail   
       mailSender.From = "rakesh.girase@thedigitalgroup.net";  
       mailSender.To = "customers@digitalgroup.com";  
       mailSender.Subject = "TestMail";  
       mailSender.Body = "This is a text mail";  
       mailSender.SendEmail();  
     }  
     catch (Exception ex)  
     {  
       fileLogger.Error("Error while Adding Invoice", ex);  
     }  
   }  
   public void Delete()  
   {  
     try  
     {  
       fileLogger.Info("Add Delete Start");  
       // Code for Delete invoice  
     }  
     catch (Exception ex)  
     {  
       fileLogger.Error("Error while Deleting Invoice", ex);  
     }  
   }  
 }  
 public interface ILogger  
 {  
   void Info(string info);  
   void Debug(string info);  
   void Error(string message, Exception ex);  
 }  
 public class FileLogger : ILogger  
 {  
   public FileLogger()  
   {  
     // Code for initialization i.e. Creating Log file with specified   
     // details  
   }  
   public void Info(string info)  
   {  
     // Code for writing details into text file   
   }  
   public void Debug(string info)  
   {  
     // Code for writing debug information into text file   
   }  
   public void Error(string message, Exception ex)  
   {  
     // Code for writing Error with message and exception detail  
   }  
 }  
 public class MailSender  
 {  
   public string From { get; set; }  
   public string To { get; set; }  
   public string Subject { get; set; }  
   public string Body { get; set; }  
   public void SendEmail()  
   {  
     // Code for sending mail  
   }  
 }  

So here is my refactoring. You will see that it is very, very different than the original improved suggested version.

  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103
104
105
 interface Invoice  
 {  
   void Add();  
   void Delete();  
 }  
 interface BetterInvoice extends Invoice  
 {  
   void Error(String msg, Exception e);  
 }  
 class InvoiceBasics implements Invoice  
 {  
   public void Add()  
   {  
      // code for adding invoice  
   }  
   public void Delete()  
   {  
      // code for deleting invoice  
   }  
 }  
 class InvoiceLogging implements BetterInvoice  
 {  
   InvoiceLogging(Invoice inner)  
   {  
      this.inner = inner;  
   }  
   public void Add()  
   {  
     fileLogger.Info("Add method Start");  
       inner.Add();  
   }  
   public void Delete()  
   {  
     fileLogger.Info("Add Delete Start");  
      inner.Delete();  
   }  
   public void Error(String msg, Exception e)  
   {  
      fileLogger.Error(msg, e);  
   }  
 }  
 class InvoiceErrorFlow implements Invoice  
 {  
   BetterInvoice inner;  
   InvoiceErrorFlow(BetterInvoice inner)  
   {  
      this.inner = inner;  
   }  
   public void Add()  
   {  
     try  
     {  
        inner.Add();  
     }  
     catch (Exception ex)  
     {  
       inner.Error("Error while Adding Invoice", ex);  
     }  
   }  
   public void Delete()  
   {  
     try  
     {  
        inner.Delete();  
     }  
     catch (Exception ex)  
     {  
       inner.Error("Error while Deleting Invoice", ex);  
     }  
   }  
 }  
 class InvoiceMailer implements Invoice  
 {  
   InvoiceMailer(Invoice inner)  
   {  
      this.inner = inner;  
   }  
   public void Add()  
   {  
      inner.Add();  
     sendMail();  
   }  
   private void sendMail()  
   {  
     mailSender.From = "rakesh.girase@thedigitalgroup.net";  
     mailSender.To = "customers@digitalgroup.com";  
     mailSender.Subject = "TestMail";  
     mailSender.Body = "This is a text mail";  
     mailSender.SendEmail();  
   }  
   public void Delete()  
   {  
      inner.Delete();  
   }  
 }  
 class InvoiceBuilder  
 {  
   public Invoice create()  
   {  
      return new InvoiceMailer(  
        new InvoiceErrFlow(  
           new InvoiceLogging(new(  
             new InvoiceBasics())));  
   }  
 }  

I bet you weren't expecting that. I have been a developer for over 30 years and it has taken some time to see software the way I do. So lets evaluate this. Is the code better? It depends.
  • The code above has separation of logging, error flow, and notification and emphasizes composition instead of inheritance.
  • The main business logic (found in InvoiceBasics) is very simple and clean.
  • More functionality can be added without disturbing everything else.
  • It is far more testable than the original.
  • It conforms more to the Single Responsibility Principle than the original.
But there is a cost. There are more classes (that do less). It is probably easier to understand what happens in the original code, where you'll have to think about it with the newly refactored code. It took more time to do than the refactored version, but you gain in testability, flexibility, etc. So you have to take all this into account. You, as the designer, have to decide how much money to spend on this code and how long you expect it to live. It is not always easy to decide.

Saturday, November 12, 2016

Becoming a Top Notch Programmer

So you want to be the best developer you can be? Warning - your career needs resemble those of a living plant more than that inert diploma sitting on your wall. Uncle Bob of Clean Coders, said that developers should spend upwards of 20 hours per week in “sharpening the saw”. I’ll let that sink in for a minute – that is half of your normal work week – on top of your real work. Now you know what you’re up against.

So here are my recommendations to anyone who wants to get better…

On Software Craftsmanship

You are paid to make decisions. One of the critical ones is how much money to spend on developing a chunk of logic. Be thoughtful about taking shortcuts because you are just pushing even bigger costs into the future. Those costs will be born by you, your colleagues, and your company. The trick is to "do the right thing" without over-engineering (over spending). Here are some great technical resources you should consider:

Clean Code by Robert Martin (also website has good video series)
Design Patterns by Gamma and Beck
Refactoring by Fowler and Beck
Look on the Pragmatic Bookshelf for others

On Polyglot Programming

Make it your goal to learn a new programming language each year, even if you aren't going to use it in your job. It will change the way you think about problems.

Really great ones to learn are: Groovy, Javascript, Scala, and Go

On Agile Practices

If you want to find a way to efficiently deal with the unknown and constantly changing demands, you need to fully understand agile.

Phoenix Project by Kim et. al. (you will have a hard time putting this book down)
Pragmatic Programmer by Hunt and Thomas
Practices of an Agile Developer by Venkat Subramaniam

On Career Planning

Don't ignore your career. Don't drift along. Even if you don't have specific goals right now, you should read up on career planning - you'll get lots of ideas. A good place to start is

Passionate Programmer by Chad Fowler

On Learning via Blogs

Spend 20 minutes each day trying to keep up with the flood of great insights and ideas by reading blogs. Here are some of my favorites.

Martin Fowler Bliki (design and abstractions)
Robert Martin’s 8th Light (software process and practices)
Jeff Atwood’s Coding Horror (hard core rubber meets the road advice)
Petri Kainulainen (software testing)
Yegor Bugayenko (software rebel and contrarian – this will get you out of your rut)
Java at DZone (great for keeping up with the latest)
Scot Hanselman’s Computer Zen (ignore the .NET stuff but lap up all the rest)
Ted Neward’s Interoperability Happens (general software, product, practices)


On Recharging Your Creative Batteries at Conferences

Nothing like mingling with your peers to recharge your batteries and to get exposed to new ideas.
Go to all the conferences that you can fit/afford.

No Fluff Just Stuff in various cities across the country including San Diego (one of my favorite and very affordable)

That's the five minute recommendation. Good luck. Grow. Prosper.

Wednesday, March 25, 2015

Avoiding Code Coverage Legacy Dilution

How do you keep your weakly tested legacy code from dragging down your coverage numbers for your new code?

If you have the luxury of excluding all the legacy code, you can have your coverage numbers reflect only the new code. You might try running two coverage calculations - one for the legacy and one for the new code. This is doable but cumbersome.

If you want to have a single coverage calculation in your build, dealing with legacy code might mean diluting your coverage numbers. Nobody is going to pay your team to go back and retrofit unit tests to match the standards of the new code.

What to do, what to do?

On my previous team, we ran into this situation. We were using JaCoCo to do coverage calculations and some of the code in our code base was user interface logic and was hard/expensive to test, so it had low coverage. All the server-side logic and shared logic had really good code coverage. We really wanted to treat them differently but did not want to open the door for developers to get lazy and go to the "lowest common denominator".

So what I did was build a tool that allowed us to express flexible coverage limits based on individual or groups of packages and/or classes per coverage metric. The tool took a set of rules as input, the current JaCoCo coverage report, and applied the rules to the metrics, signalling an error if any of the rules were violated.

The simplest rules file contents is shown below. Since this is CSV-based, the first row shows the static column headers. For these examples, we will only talk about LINE coverage, but obviously other coverage metrics are possible.

This single rule says that all packages in-aggregate, with all classes in each package in-aggregate, should have a minimum 70% line coverage.

PACKAGE, CLASS, ABC_MISSED, ABC_COVERED, LIMIT
*, *, LINE_MISSED, LINE_COVERED, 0.7

The following rules show how you can special-case a single class for its own coverage limit of 40%. The important thing here is that the special class follows one rule and all the rest of the code follows the other rule.

PACKAGE, CLASS, ABC_MISSED, ABC_COVERED, LIMIT
com.acme, FooBar, LINE_MISSED, LINE_COVERED, 0.4
*, *, LINE_MISSED, LINE_COVERED, 0.7

As a final example, we will specify that all the classes in the special apex package should in-aggregate, have a 50% coverage.

PACKAGE, CLASS, ABC_MISSED, ABC_COVERED, LIMIT
com.acme, FooBar, LINE_MISSED, LINE_COVERED, 0.4
com.apex, *, LINE_MISSED, LINE_COVERED, 0.5
*, *, LINE_MISSED, LINE_COVERED, 0.7

I will be working on and open sourcing a coverage rules tool very soon that will follow rules like those above (and a few more).

Sunday, January 18, 2015

The Greenest of Fields

A greenfield project is a brand new project that is not subject to any legacy requirements and importantly does not deal with legacy code. The opportunity to work on a greenfield project does not come along very often and is generally viewed as a real opportunity by developers. In the excitement, it is tempting to focus on the new design and the new code.

Beyond the technology decisions of the new project, there are things to consider that will have an under appreciated impact on the project. These issues are the subject of this blog posting. If you ignore these issues or make the wrong decision, changing you mind could be very expensive.

Unit Testing and Code Coverage. You should decide before any code is written whether you are going to establish unit tests and track code coverage in your build. Anyone who has a legacy code base and wants to go back and unit test will tell you that there isn't a business in the world that will pay your developers to go back and do this - it is cost prohibitive. Not only that, the existing code base can affect your overall coverage numbers should you start trying to cover you new code and ignore your old code. (I'll actually discussion how to manage this in a forthcoming posting).

Coding Conventions. If your project is to adhere to coding conventions, they should be established early on. Depending on how detailed the conventions are and whether there is any tool support for transforming code, this could be expensive to retrofit once the project has a significant code base. The difficulties are compounded because code that is written to be testable is different (usually better designed) than code that is not, so you wouldn't be able to unit test legacy code without significant refactoring costs.

Language-specific Best Practices. Well beyond issues of formatting, some languages have best practices conventions that should be established early on. An example is "const correctness" in C++. Anyone who has tried to go back and retrofit "const correctness" in a code base knows how invasive this change can be. An effort like this can end up touching almost all of the classes in your project. Of particular insidiousness is that a casual code inspection often does not make the scope of such changes apparent.

Internationalization. On our projects, we always consider internationalization when we write our code, even if it is not an explicit requirement for the project. There are tools that can help sift through source code and refactor the string literals out into a separate module. This is helpful and better than nothing, but it is not as neat as doing this as you write your code. Additionally there are often considerations that make this one-size-fits-all approach suboptimal. For example, in our projects, we allow the logging (and debug information), which frequently is done on the serverside, to remain in English. We use the locale associated with the current session to determine what language the user interface shows to the end user.

Code Rules. There is a lot of tool support these days to analyze source code and determine its adherence to coding rules. A popular tool like this is FindBugs (part of Sonar). Use of this tool should be done very early on. Anyone who has turned this tool onto an existing code base knows the potentially thousands of issues/violations that can be raised. Although the rules used to generate violations can be tuned, it is far better to do this with a very small code base than a large. We turned this tool onto a large code base (say 500K LOC) and had to limit our efforts to address only critical and major issues because of the large volume. So do yourself a favor and activate this tool on all check-ins starting on day zero.

Database Content. This issue is subtle. Our application uses a backend database and to support our functional tests, we stand up test data. The data was captured as a dump. So over time, developers added test data, got their tests going, then took a new dump. The problem with this is that very quickly, nobody could explain why a particular piece of data was in the database. It had become a big tarball of opaque, but required data. Avoid this situation at all costs. You should manage your data just like you do your source code. Each addition to the test data should be identifiable and tracked. Ideally you could stand up a new test database based solely on these individual descriptions. (I'll discuss this more in a forthcoming blog entry).

Technical Debt. The urgency in establishing identification of technical debt is not as high as the other issues identified here. Technical debt is incremental and, for the most part, its cost is incremental. Delaying dealing with technical debt is a mistake for many reasons. Ideally it would be identified, tracked, and continually addressed from day zero. Doing so establishes the expectation on your development team that taking shortcuts is not free of cost and is not invisible. It will cost you in the future at some point.

Hopefully these issues will give you something to consider at your next greenfield opportunity. If you have any other ideas about issues that should be in almost all projects and are hard to retrofit, please leave comments.