Allow integrationTests to be a string

[rohangpta]

Allow integrationTests to be a string to enable passing in a commit message flag which would (hopefully) compile down to a boolean.

The idea is to use something like this "!contains(github.event.head_commit.message, '[skip int]')" as the argument to integrationTests in individual app config

[codecov]

Codecov Report

Merging #157 (2f73812) into master (272b897) will increase coverage by 2.64%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master      #157      +/-   ##
===========================================
+ Coverage   97.35%   100.00%   +2.64%     
===========================================
  Files          16        15       -1     
  Lines         265       169      -96     
  Branches       73        14      -59     
===========================================
- Hits          258       169      -89     
+ Misses          5         0       -5     
+ Partials        2         0       -2     

Flag	Coverage Δ
kittyhawk	?
kraken	100.00% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cdk/kraken/src/docker.ts	100.00% <ø> (ø)
cdk/kraken/src/labs-application.ts	100.00% <ø> (ø)
cdk/kittyhawk/src/cronjob.ts
cdk/kittyhawk/test/utils.ts
cdk/kittyhawk/src/application/django.ts
cdk/kittyhawk/src/deployment.ts
cdk/kittyhawk/src/application/base.ts
cdk/kittyhawk/src/ingress.ts
cdk/kittyhawk/src/index.ts
cdk/kittyhawk/src/application/redis.ts
... and 23 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 713bdf8...2f73812. Read the comment docs.

[joyliu-q]

Overall looks good, minor nits. When you're adding respective kraken changes to products, just make sure to enforce integration testing in the main branch (e.g. in config, if the branch is main, even if the commit message says skip, it should not skip).

[joyliu-q]

I feel like noPublish having the possibility of being a string or a boolean is a little unintuitive.

Maybe we can change the comments that specifies noPublish is a "no-publish condition", or there's a better name for this variable.

Also, since it can be a string AND is optional, we can simplify the type to just be a noPublish?: string; and check if noPublish is undefined (truthy/falsey instead of true/false directly).

[rohangpta]

Wouldn't this require a fair amount of refactoring in other places? Like we'd have to change noPublish to "true" or arbitrary truthy string value from true everywhere we specify it right?

IMO can stick to this but include better comments if ^ turns out to be true?

[rohangpta]

Also stricter typing might be slightly more unintuitive but lead to fewer errors down the line if we're passing around config, accidentally passing around undefined could be problematic.

[joyliu-q]

Another consideration is having non-confusing typing. Either make a boolean disabling publishing, or make a variable specifying the conditions to disable publishing. If you feel the need to preserve a simple true/false interface, we can introduce this feature as a separate variable.

I took a look at existing code using a defined noPublish and the only place is a test case, which means this would not be a breaking change. Even if it was used in prod, for this change to apply to a product, we would need to bump kraken's version in package.json.

[rohangpta]

What would you lean towards for the default value of integrationTests if I want it to be enabled/disabled? Do you tink it's ok if convention is to set integrationTests to something like "true" as opposed to true in, for example, this file?

I'd be ok with that and just using truthy/falsey conditioning if you think it's fine!

[joyliu-q]

Same as above.