-
Notifications
You must be signed in to change notification settings - Fork 4.5k
BEAM-78 - First step of the renaming: Maven coordonates #46
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
Conversation
examples/pom.xml
Outdated
| <version>1.5.0-SNAPSHOT</version> | ||
| <groupId>org.apache.beam</groupId> | ||
| <artifactId>parent</artifactId> | ||
| <version>1.5.0-incubating-SNAPSHOT</version> |
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.
Let us use 0.0.1 to indicate the fresh start and expected churn. It is also quite handy to have a build number (perhaps YYYYmmddMMHHSS) associated with snapshots.
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.
Wouldn't it make sense to keep the Dataflow version number? Essentially, this is a fork and we're not starting from zero.
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.
Even though I suggested it, I don't feel too strongly about using 0.0.1, I just want to maintain good semantic versioning. So another good choice is 2.0.0-SNAPSHOT.
For getting a version to someone before the first stable release, we can offer a 0.x.y release or a 2.0.0-SNAPSHOT pre-release, with similar assurance of quality and compatibility. Whatever works best for everyone and the incubating process.
I notice that there is a suggestion on the technical vision doc that we have a release plan. That would drive the decision.
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.
@kennknowles I'm all for sensible semantic versioning but the document only states the versioning for releases. In the Maven world, SNAPSHOT indicates a non-released minor version. Usually, it doesn't carry the "patch" part of the release version. The reasoning behind that is to snapshot the minor version during development and backport patches into patch releases.
Minor Release: 0.1.0
Patch release: 0.1.1
Unreleased minor version: 0.2-SNAPSHOT
Unreleased major version: 1.0-SNAPSHOT
Major release: 1.0.0
The design doc suggests to start from 0.x.y. So I guess it makes sense to stick to the initial proposal then and make it 0.1-SNAPSHOT. Perhaps we should move the discussion to the mailing list :)
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.
Agree with Max. 0.1-incubating-SNAPSHOT makes sense. We should discuss this on the mailing list anyway.
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.
Sounds great. I only say 0.x.y to please tools that require two dots :-)
Were we going to get commits@ subscribed to the firehose of all comments on all PRs? I think that would be "helpful" for spreading knowledge about developments, and make it easy to pull things from commits@ to dev@
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.
(see my comment on the dev mailing list)
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.
Let me update the PR with 0.1.0-incubating-SNAPSHOT then.
|
I'll take a look shortly. R: @davorbonaci |
examples/pom.xml
Outdated
| <project xmlns="http://maven.apache.org/POM/4.0.0" | ||
| xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> |
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.
What's the rationale being wrapping change? It is easier to read 3 shorter lines, than one really long one.
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.
Good point. I will use one line. By habit, I used wrapping because we had issues in the release and version plugins. But it's now fixed, so, I will use one line.
|
I took a quick peek; left a few comments. Thanks JB -- this is a clear improvement. |
|
Updated PR including Davor's comments. I will now start the package renaming. |
|
There's a build break regarding archetypes. Otherwise, LGTM. |
|
I have fixed up this pull request to have the build pass (hopefully) and merged it. Thanks JB! |
|
Thanks Davor, much appreciated ! |
Identify side inputs by (PTransform id, local name, PCollection id)
[BEAM-3900] Auto formating, CheckStyle and FindBugs fixes.
The original restriction is maintained when splitting a restriction.
No description provided.