Skip to content

[BEAM-154] Use dependencyManagement and pluginManagement to keep all … #267

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
Closed
jbonofre wants to merge 1 commit into from
Closed

[BEAM-154] Use dependencyManagement and pluginManagement to keep all … #267

jbonofre wants to merge 1 commit into from

Conversation

@jbonofre
Copy link
Member

@jbonofre jbonofre commented Apr 29, 2016

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

Use of dependencyManagement and pluginManagement to align versions between modules.

There's checkstyle errors in several modules, I will fix in new commits.

kennknowles
kennknowles reviewed Apr 29, 2016
View reviewed changes
pom.xml Outdated
<executions>
<execution>
<id>attach-sources</id>
<phase>compile</phase>
Copy link
Member

@kennknowles kennknowles Apr 29, 2016

Choose a reason for hiding this comment

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

Can we attach this to a later phase, such as package, to speed up compile?

Copy link
Member Author

@jbonofre jbonofre Apr 29, 2016

Choose a reason for hiding this comment

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

Sure, let me do it.

@kennknowles
Copy link
Member

kennknowles commented Apr 29, 2016

One little thought: if we are moving to one common checkstyle.xml then could it just be in the top-level parent project, which is the only place it is referenced? It took me a second to figure out what is going on with sdks/java/build-tools, since that module has nothing to do with SDKs.

@jbonofre
Copy link
Member Author

jbonofre commented Apr 29, 2016

It's what I'm planning once the checkstyle errors will be fixed (I'm working on it).

Actually, I plan to define some plugin by default, and a BOM for the dependencies. This is the first step.

@kennknowles
Copy link
Member

kennknowles commented Apr 29, 2016

LGTM to get the big picture setup and forward-fix any little things, but I think Davor will probably want to take a close look.

R: @davorbonaci

@jbonofre
Copy link
Member Author

jbonofre commented Apr 29, 2016

Yup, thanks for the review @kennknowles ! Much appreciated !

davorbonaci
davorbonaci reviewed May 1, 2016
View reviewed changes
pom.xml Outdated
<dependencyManagement>
<dependencies>

<!-- Beam SDK -->
Copy link
Member

@davorbonaci davorbonaci May 1, 2016

Choose a reason for hiding this comment

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

I'd probably remove these comments, because we aren't consistent (some artifacts have it and others don't), and they aren't particularly informative.

I'd leave comment at the beginning of the testing scope section, and those that describe the exclusions.

Copy link
Member Author

@jbonofre jbonofre May 2, 2016

Choose a reason for hiding this comment

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

It makes sense, let me do it.

@davorbonaci
Copy link
Member

davorbonaci commented May 1, 2016

This is a great improvement!

Separately, I think we should think about build speed and potentially adjust plugins to make sure things are optimized for the development workflow, while the CI tools execute everything. We can defer this to a later point.

mxm
mxm reviewed May 2, 2016
View reviewed changes
runners/flink/examples/pom.xml Outdated
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-source-plugin</artifactId>
</plugin>
Copy link
Contributor

@mxm mxm May 2, 2016

Choose a reason for hiding this comment

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

I think it's enough to include this in the root Flink Runner pom.

Copy link
Member Author

@jbonofre jbonofre May 2, 2016

Choose a reason for hiding this comment

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

Good idea, let me update this way.

@jbonofre
Copy link
Member Author

jbonofre commented May 2, 2016

Rebased and updated according to the comments.

@davorbonaci
Copy link
Member

davorbonaci commented May 2, 2016

LGTM, modulo resolution of the comment regarding google-http-client-jackson's exclusion of transitive dependencies.

@mxm
Copy link
Contributor

mxm commented May 3, 2016

+1 from me as well. Perhaps we could add the minimum version check to the project pom but it is not super pressing.

@jbonofre
Copy link
Member Author

jbonofre commented May 3, 2016

Agree: I will put the enforcer on the main parent pom.

@jbonofre
Copy link
Member Author

jbonofre commented May 3, 2016

Rebased and updated according to the comments. FYI, checkstyle and enforcer will be submitted in other PRs.

@davorbonaci
Copy link
Member

davorbonaci commented May 3, 2016

LGTM. Merge in progress.

@jbonofre
Copy link
Member Author

jbonofre commented May 3, 2016

Awesome thanks !

@asfgit asfgit closed this in 6260cc1 May 3, 2016
@jbonofre jbonofre deleted the DEPMANAGEMENT branch May 8, 2016 06:17
iemejia pushed a commit to iemejia/beam that referenced this pull request Jan 12, 2018
This closes apache#267
41061fc
mareksimunek pushed a commit to mareksimunek/beam that referenced this pull request May 9, 2018
@dmvk
Merge pull request apache#267 from seznam/upgrade_kafka
6771298 
Upgrade kafka to 1.0.0
damccorm pushed a commit that referenced this pull request Dec 19, 2022
@dannymartinm @elink21 @elink22
[GitHub Actions] - Updates in Typescript Tests to runs-on Self-hosted…
ae76792 
… runners (#23164)

* Updated typescript_tests workflow (#178)

Co-authored-by: Elias Segundo <elias.segundo@luisrazo.local>

* Added master changes in typescript_tests to avoid merge conflicts

* Updated typescript_xlang_tests job to runs-on self-hosted ubuntu runner.

* Switching trigger to pull_request (#267)

* Switching trigger to pull_request

* Removing ref from checkout

Co-authored-by: Elias Segundo Antonio <eliassegundo.segundo@gmail.com>
Co-authored-by: Elias Segundo <elias.segundo@luisrazo.local>
Co-authored-by: elink22 <103056145+elink22@users.noreply.github.com>
lostluck pushed a commit to lostluck/beam that referenced this pull request Dec 22, 2022
@dannymartinm @elink21
@elink22 @lostluck
[GitHub Actions] - Updates in Typescript Tests to runs-on Self-hosted…
04913d2 
… runners (apache#23164)

* Updated typescript_tests workflow (apache#178)

Co-authored-by: Elias Segundo <elias.segundo@luisrazo.local>

* Added master changes in typescript_tests to avoid merge conflicts

* Updated typescript_xlang_tests job to runs-on self-hosted ubuntu runner.

* Switching trigger to pull_request (apache#267)

* Switching trigger to pull_request

* Removing ref from checkout

Co-authored-by: Elias Segundo Antonio <eliassegundo.segundo@gmail.com>
Co-authored-by: Elias Segundo <elias.segundo@luisrazo.local>
Co-authored-by: elink22 <103056145+elink22@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jbonofre @kennknowles @davorbonaci @mxm @tomwhite @amitsela