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.

Sign up for GitHub

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

Jump to bottom

Allow integrationTests to be a string #157

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Allow integrationTests to be a string #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b4d634
allow integrationTests to be string
rohangpta Jul 10, 2022
2f73812
modify downstream flag
rohangpta Jul 10, 2022
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
4 changes: 2 additions & 2 deletions cdk/kraken/src/docker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,11 @@ export interface DockerPublishJobProps {
cache?: boolean;

/**
* If enabled, do not publish docker image to Docker Hub.
* If evaluates to true, do not publish docker image to Docker Hub.
* Instead upload as an artifact.
* @default false
*/
noPublish?: boolean;
noPublish?: boolean | string;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@joyliu-q joyliu-q Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

}

/**
Expand Down
4 changes: 2 additions & 2 deletions cdk/kraken/src/labs-application.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ export interface LabsApplicationStackProps {
frontendPath?: string;

/**
* If true, run integration tests using docker-compose.
* If evaluates to true, run integration tests using docker-compose.
* @default false
*/
integrationTests?: boolean;
integrationTests?: boolean | string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.


/**
* Base name of the docker images to publish.
Expand Down