Skip to content

Setting Up the Release Process #3

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

Merged
merged 55 commits into from
Apr 17, 2017
Merged

Setting Up the Release Process #3

merged 55 commits into from
Apr 17, 2017

Conversation

hiranya911
Copy link
Contributor

  • Configure the release process using Maven and Nexus plugins.
  • Remove the firebase.it.url system property used in the integration tests. The database URL is now inferred from the project ID value in the provided certificate file.
  • Reading SDK version from a resource file that is automatically updated by Maven at build time.

Hiranya Jayathilaka and others added 30 commits April 5, 2017 19:12
* Reformatted all source files
* Separated unit and integration tests
* Added maven checkstyle plugin
* Got the basic build and unit tests working
* Scrubbed the codebase for private keys, certificates etc.
Copy link

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

Basically LGTM, but I don't know maven stuff at all so if you want a sanity-check there you'll need to find somebody else. Maybe ask depoll@ to look or recommend a reviewer.

@@ -0,0 +1,130 @@
# Contributing | Firebase Admin Python SDK

Choose a reason for hiding this comment

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

Python? :-P

(same thing in a few places; please search this file for "python" and fix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops :)

Fixed.

command to invoke the integration test suite:

```
mvn verify -Dfirebase.it.certificate=path/to/your/serviceAccount.json

Choose a reason for hiding this comment

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

Why "certificate" instead of "serviceAccount" or something? I'm not really sure if "certificate" is appropriate.


Make sure to specify the correct path to your downloaded service account key file as the
`firebase.it.certificate` system property. This command will invoke both unit and integration test
suites. To execute only the integration tests, run the command as follows:

Choose a reason for hiding this comment

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

I don't feel strongly, but why would you only want to run the integration tests? It seems more useful to run only the unit tests (since they're presumably faster and would work even without an internet connection). But I think I'd be inclined to just direct folks to run everything (and remove this section).

@hiranya911 hiranya911 requested review from depoll and vikrum April 14, 2017 18:11
@jwngr jwngr removed their request for review April 14, 2017 22:44
…tps://github.com/google/guava/wiki/Release21); Excluding the transitive dependency on Guava 17 from Google API client to prevent having 2 versions of Guava in the classpath
Copy link

@depoll depoll left a comment

Choose a reason for hiding this comment

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

This basically LGTM. It's still probably worth finding someone (e.g. vikrum@) to review the maven bits, but we probably don't need to block on that.

@@ -94,7 +191,7 @@
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>21.0</version>
Copy link

Choose a reason for hiding this comment

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

Did we step back to an older version of Guava?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. As it turns out 21.0 only works on Java 8. Their documentation recommends using 20.0 for Java 7 compatibility: https://github.com/google/guava/wiki/Release21

@hiranya911
Copy link
Contributor Author

I will merge this PR as it is now. I've tested the Maven stuff by running the release plugin in dryRun mode, and also by pushing a snapshot release to Nexus. There might be other minor glitches, which I will encounter during the next release. I can fix them at that time, and also update our release process docs accordingly.

@hiranya911 hiranya911 merged commit ebddd1b into master Apr 17, 2017
@hiranya911 hiranya911 deleted the hkj-release-process branch April 17, 2017 20:05
Copy link

@vikrum vikrum left a comment

Choose a reason for hiding this comment

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

pom.xml LGTM.

@vikrum
Copy link

vikrum commented Apr 17, 2017

I imagine we might need to iterate on the POM once we attempt to stage a release (semi-expected/normal given interacting with their system.)

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.

4 participants