-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
gradle: create a separate sub build for all connectors #11833
gradle: create a separate sub build for all connectors #11833
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments. See what you think. Overall 👍
@@ -28,8 +28,8 @@ if (!System.getenv().containsKey("SUB_BUILD")) { | |||
} else { | |||
def subBuild = System.getenv().get("SUB_BUILD") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that this is getting used in more places, it might be nice to move it up a level in visibility, so the below doesn't have to repeat itself much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please clarify? This sub-build is only defined in this settings file and is already at top level of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant you could have move the variable definition def subBuild = System.getenv().get("SUB_BUILD")
to higher up in the file and then used it below instead of System.getenv().containsKey("SUB_BUILD")
around line 79 below. Not a huge deal.
@@ -28,8 +28,8 @@ if (!System.getenv().containsKey("SUB_BUILD")) { | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment just above here is out of date now. Add ALL_CONNECTORS
to the list. It's minor but I think just CONNECTORS
would be fine too and maybe more intuitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use ALL_CONNECTORS
to distinguish it from CONNECTORS_BASE
.
NIT: I think it worth sharing that in #dev after merging so the team know that they can a new alias once merge. |
What
The
/test connectors=all
command is selecting and running octavia-cli's integrationTest but it should not.How
Create a separate sub-build for all connectors that is not including octavia-cli.