Add daily only option

[stephen-soltesz]

This change adds a new gardener datatype configuration option for daily_only: <bool>. Default behavior is unchanged.

This change refactors the config package interface to use standard return type and error propagation, removes a package flag in favor of defining this in the main package, and completes unit test coverage of that package.

This change will ultimately allow us to set the pcap processing to "daily only" to prevent the excess reprocessing for historical pcap data.

This change obsoletes the change started in:

• https://github.com/m-lab/etl-gardener/pull/352

Part of a sequence of changes leading to:

• https://github.com/m-lab/etl-gardener/issues/349

This change is 

[coveralls]

Pull Request Test Coverage Report for Build 3382

• 56 of 60 (93.33%) changed or added relevant lines in 4 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.8%) to 73.321%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
job-service/job-service.go	32	36	88.89%
<!--	Total:	56	60	93.33%	-->

Files with Coverage Reduction	New Missed Lines	%
job-service/job-service.go	1	87.72%
<!--	Total:	1		-->

Totals
Change from base Build 3374:	0.8%
Covered Lines:	1583
Relevant Lines:	2159

---

💛 - Coveralls

[SaiedKazemi]

cmd/gardener/gardener.go line 284 at r1 (raw file):

  // TODO Once the legacy deployments are turned down, this should move to head of main().
  gcfg, err := config.ParseConfig(*configPath)
  rtx.Must(err, "Failed to parse config: %q", *configPath)

Per readability guidelines, error strings should not be capitalized.

[SaiedKazemi]

config/config.go line 56 at r1 (raw file):

  if err != nil {
      return nil, err
  }

nit: just curious why the shorter if err := readFile(name, g) { isn't preferred here.

[SaiedKazemi]

config/config_test.go line 41 at r1 (raw file):

      },
      {
          name:    "error-no-file",

nit: does it make sense to call this "error-no-file-name" to make it more distinguishable form "error-no-such-file"?

[SaiedKazemi]

A couple of minor suggestions, but other looks good.

[SaiedKazemi]

cmd/gardener/gardener.go line 284 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Since this is `rtx.Must`, I think it may be preferable to think of this like a `log.Fatal` rather than an error. This constructs the final message reported before exiting (and includes the given error as string). Should the same policy apply to this last log message as we apply to error strings?

In the case of rtx.Must() it makes sense to capitalize. But now one has to know what rtx.Must() does and that's why I prefer using log.Fatal() (but I guess it's personal choice).

[SaiedKazemi]

config/config.go line 66 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

In this case we actually want `f` not to be locally scoped.

I actually deleted this comment because I saw we need f. Not sure why it wasn't removed.

[SaiedKazemi]

config/config_test.go line 41 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Clearer is better. I've updated both names, here and below.

Thanks.

[SaiedKazemi]

[stephen-soltesz]

(I'll revert the config change before merging) - but wanted to test overnight and over the weekend (if it appears to work) with all types as daily_only: true.

[stephen-soltesz]

Looks like the new behavior WAI over night. I'll leave it over the weekend and expect to be able to revert the config change and merge on Monday.

[stephen-soltesz]

This appears to reliably work as intended. I'll revert the config changes (so no types are daily_only:true (but can be in the future)) and merge.

[stephen-soltesz]

FYI: @cristinaleonr - for pcap in the near future.