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.

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

feat(build_release): Use gradle to publish builds instead of manual c… #1430

Merged
merged 1 commit into from Feb 23, 2017

Conversation

jtk54
Copy link
Contributor

@jtk54 jtk54 commented Feb 16, 2017

…opy.

@@ -211,28 +212,62 @@ def determine_gradle_root(self, name):
gradle_root = name if name != 'spinnaker' else self.__project_dir
return gradle_root

def start_build_target(self, name, target):
"""Start a subprocess to build the designated target.
def start_candidate_release(self, name):

Choose a reason for hiding this comment

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

Use terminology we established earlier.

candidate releases dont start until builds have been verified.
The build part is initiating an unverified build.

However this function is merely building a subsystem.
Where it falls into the release process is a matter the caller's context.
Dont assume that here. This is just the primitive operator that the
release process is composed with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I can rename the function but unfortunately the gradle task that is executed has to be either candidate or final.

print 'Building and publishing {name}...'.format(name=name)
return BackgroundProcess.spawn(
'Building and publishing {name}...'.format(name=name),
'cd "{gradle_root}"; ./gradlew {extra} candidate'.format(

Choose a reason for hiding this comment

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

I dont like "candidate" here. Or if it is we need to change our terminology. However calling these things candidates prematurely causes confusion later as we discovered earlier on defining the process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

candidate is the Nebula gradle task name. The only other choice that behaves the way we want is final at the moment, and that doesn't seem better. I'm not too sure about our ability to rename these.

BackgroundProcess
"""
jarRepo = self.__options.jar_repo
parts = self.__options.bintray_repo.split('/')

Choose a reason for hiding this comment

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

Do these come from gradle properties?
I'm ok with environment variables.
I thought the current gradle plugins used properties.

Copy link
Contributor Author

@jtk54 jtk54 Feb 16, 2017

Choose a reason for hiding this comment

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

These are flags passed into this script, which can be populated by environment variables. We then use them as properties in the gradle task (which are also flags/arguments).

@@ -529,6 +511,14 @@ def init_argument_parser(cls, parser):
'--bintray_repo', default='',
help='Publish to this bintray repo.\n'
'This requires BINTRAY_USER and BINTRAY_KEY are set.')
parser.add_argument(

Choose a reason for hiding this comment

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

jar repo shouldnt have a default.
Maybe this should be a property?

Is this jar repo write only or is it also read?
When other people run this script, they shouldnt publish to the official repo, but should be able to read form it rather than building everything themselves. But if they do build things, they should read those from their repo. So this should be for writing, and there should be a path set somewhere that this has precedence of the existing external path for jar files in this repo but not elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First part makes sense. I'm confused at the second.

The jar repo is read/write as long as you have a valid key with permission to that repo or it is public. I agree that everyone shouldn't be able to publish to the official repo, but I don't understand why users would want to read from the official repo in this case. This is for building from source, not copying upstream, prebuilt artifacts. As you put it, this is for building artifacts and writing to bintray_repo and jar_repo. Consuming artifacts from those repos is a separate concern.

Choose a reason for hiding this comment

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

I could be confused.

If I am building jars I put them in this repository. Fine. But I dont build every jar I depend on, only the ones defined in this [git] repository. I dont know how this works under the covers, but isnt clear how the precedence and scoping works (or how things get removed so that I can use official external jars even if I may have built those projects myself earlier so the same versions are in my repo (built were built with a different tool chain).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This jar_repo is the target repo for the microservice jars, not the dependency jars. For example, the jars produced from testing this script: https://bintray.com/spinnaker-team/jacobkiefer-jars. My understanding is all the dependency jars are downloaded to a local repository when the microservices are being built, and are pulled at whatever version is specified in build.gradle.

@ewiseblatt
Copy link

ewiseblatt commented Feb 16, 2017 via email

@jtk54 jtk54 force-pushed the gradle-publish branch 3 times, most recently from 22f3b65 to cccec5b Compare February 16, 2017 23:37
@ewiseblatt
Copy link

ewiseblatt commented Feb 16, 2017 via email

@jtk54
Copy link
Contributor Author

jtk54 commented Feb 23, 2017

Kork would need to be built and published and the up-to-date version specified in spinnaker dependencies. If you wanted to include your own version of kork in the microservices, then you'd need to modify each build.gradle to point to a different target maven repository.

help='Publish produced jars to this repo.\n'
'This requires BINTRAY_USER and BINTRAY_KEY are set.')
parser.add_argument(
'--build_number', default='',

Choose a reason for hiding this comment

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

Is this required? What's the default or is it left out if not specified?

@@ -185,6 +185,7 @@ def __init__(self, options):
self.__background_processes = []

os.environ['NODE_ENV'] = os.environ.get('NODE_ENV', 'dev')
self.__build_number = options.build_number or os.environ.get('BUILD_NUMBER', '')

Choose a reason for hiding this comment

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

set the argparser default to os.environ.get('BUILD_NUMBER', '')

extra_args = [
'--stacktrace',
'-Prelease.useLastTag=true',
'-PbintrayPackageBuildNumber={number}'.format(number=self.__build_number),

Choose a reason for hiding this comment

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

build_number could be empty. is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The gradle task interprets an empty string as 'no build number' and names the package accordingly.

Copy link

@ewiseblatt ewiseblatt left a comment

Choose a reason for hiding this comment

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

LGTM

@jtk54 jtk54 merged commit 66d0952 into spinnaker:master Feb 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants