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.

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

[ZEPPELIN-1695] Enforce pom.xml style while removing useless, unapplied conf #1662

Closed
wants to merge 5 commits into from
Closed

[ZEPPELIN-1695] Enforce pom.xml style while removing useless, unapplied conf #1662

wants to merge 5 commits into from

Conversation

1ambda
Copy link
Member

@1ambda 1ambda commented Nov 21, 2016 •
edited

What is this PR for?

First of all, this PR doesn't affect on runtime application behaivor and existing build processes. Just adding validation and styling

The main goal of this PR is to keep consistency among multiple pom.xml files

  • added xml-maven-plugin to enforce valid maven conf and xml code style (included in maven verify phase)
  • while removing 2 invalid combine.children conf (they are invalid, useless. see commit messages)
  • apply 2 space indent according to maven style guide

For reviewers,

most code modification is due to 4064057 (re-indent commit), so skipping the commit will be helpful to review this.

What type of PR is it?

[Improvement]

Todos

  • - Add maven xml plugin
  • - Reindent according to maven style guide

What is the Jira issue?

ZEPPELIN-1695

How should this be tested?

CI will do that. Also you can type mvn xml:validate usually it takes 10+- seconds.

[INFO] Reactor Summary:
[INFO]
[INFO] Zeppelin ........................................... SUCCESS [  2.444 s]
[INFO] Zeppelin: Interpreter .............................. SUCCESS [  0.048 s]
[INFO] Zeppelin: Zengine .................................. SUCCESS [  0.029 s]
[INFO] Zeppelin: Display system apis ...................... SUCCESS [  0.025 s]
[INFO] Zeppelin: Spark dependencies ....................... SUCCESS [  0.038 s]
[INFO] Zeppelin: Spark .................................... SUCCESS [  0.040 s]
[INFO] Zeppelin: Markdown interpreter ..................... SUCCESS [  0.022 s]
[INFO] Zeppelin: Angular interpreter ...................... SUCCESS [  0.013 s]
[INFO] Zeppelin: Shell interpreter ........................ SUCCESS [  0.019 s]
[INFO] Zeppelin: Livy interpreter ......................... SUCCESS [  0.018 s]
[INFO] Zeppelin: HBase interpreter ........................ SUCCESS [  0.014 s]
[INFO] Zeppelin: Apache Pig Interpreter ................... SUCCESS [  0.016 s]
[INFO] Zeppelin: PostgreSQL interpreter ................... SUCCESS [  0.012 s]
[INFO] Zeppelin: JDBC interpreter ......................... SUCCESS [  0.012 s]
[INFO] Zeppelin: File System Interpreters ................. SUCCESS [  0.025 s]
[INFO] Zeppelin: Flink .................................... SUCCESS [  0.048 s]
[INFO] Zeppelin: Apache Ignite interpreter ................ SUCCESS [  0.013 s]
[INFO] Zeppelin: Kylin interpreter ........................ SUCCESS [  0.010 s]
[INFO] Zeppelin: Python interpreter ....................... SUCCESS [  0.011 s]
[INFO] Zeppelin: Lens interpreter ......................... SUCCESS [  0.015 s]
[INFO] Zeppelin: Apache Cassandra interpreter ............. SUCCESS [  0.036 s]
[INFO] Zeppelin: Elasticsearch interpreter ................ SUCCESS [  0.014 s]
[INFO] Zeppelin: BigQuery interpreter ..................... SUCCESS [  0.012 s]
[INFO] Zeppelin: Alluxio interpreter ...................... SUCCESS [  0.011 s]
[INFO] Zeppelin: Scio ..................................... SUCCESS [  0.026 s]
[INFO] Zeppelin: web Application .......................... SUCCESS [  1.193 s]
[INFO] Zeppelin: Server ................................... SUCCESS [  0.015 s]
[INFO] Zeppelin: Packaging distribution ................... SUCCESS [  0.022 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 4.678 s
[INFO] Finished at: 2016-11-22T00:24:32+09:00
[INFO] Final Memory: 27M/703M
[INFO] ------------------------------------------------------------------------

Questions:

  • Does the licenses files need update? NO
  • Is there breaking changes for older versions? NO
  • Does this needs documentation? NO

@1ambda 1ambda changed the title [WIP] CI Test for changed maven configuration [ZEPPELIN-1695] Enforce pom.xml style while removing useless, unapplied conf Nov 22, 2016
@bzz
Copy link
Member

bzz commented Nov 24, 2016 •
edited

@1ambda thank you for great effort! It's a bit hard to review as re-formatting is mixed with pom.xml modifications.

How hard would be to have 2 separate PRs - one with improvements, and another one, with re-formatting?

Other than that - it looks great to me.

@1ambda
Copy link
Member Author

1ambda commented Nov 24, 2016

@bzz I couldn't consider the reviewer's point of view. I will split this! Thanks again :)

@1ambda
feat: Add xml-maven-plugin to validate pom.xml
4418cad
@1ambda
fix: Remove useless combine.chdilren
03579e7 
[ERROR] Failed to execute goal org.codehaus.mojo:xml-maven-plugin:1.0.1:validate (default-cli) on project zeppelin: While parsing /Users/lambda/github/apache-zeppelin/zeppelin-master/pom.xml, at file:/Users/lambda/github/apache-zeppelin/zeppelin-master/pom.xml, line 430,  column 52: cvc-complex-type.3.2.2: Attribute 'combine.children' is not allowed to appear in element 'configuration'. -> [Help 1]
@1ambda
fix: Remove useless encoding options
1142b46
@1ambda
fix: Use _tools/maven-4.0.0.xsd
f858a16
@1ambda
fix: Remove useless combine.children=append
2d8d7d2 
Since forkMode is a single element option, we don't need `combine.children` option.

- ref: https://maven.apache.org/pom.html

```
[ERROR] Failed to execute goal org.codehaus.mojo:xml-maven-plugin:1.0.1:validate (default-cli) on project zeppelin-zengine: While parsing /Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-zengine/pom.xml, at file:/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-zengine/pom.xml, line 253,  column 50: cvc-complex-type.3.2.2: Attribute 'combine.children' is not allowed to appearin element 'configuration'. -> [Help 1]
```

maven effective pom shows same result after removing that option

```
// after removed
<plugin>
        <artifactId>maven-surefire-plugin</artifactId>
        <version>2.17</version>
        <executions>
          <execution>
            <id>default-test</id>
            <phase>test</phase>
            <goals>
              <goal>test</goal>
            </goals>
            <configuration>
              <forkMode>always</forkMode>
              <argLine>-Xmx2g -Xms1g -Dfile.encoding=UTF-8</argLine>
            </configuration>
          </execution>
        </executions>
```
@1ambda 1ambda force-pushed the feat/check-pom-file-is-well-formed branch from 8285048 to 2d8d7d2 Compare November 24, 2016 05:47
@bzz
Copy link
Member

bzz commented Nov 24, 2016

Thanks!

Looks great to me, will merge to master as soon as CI is green, if there is no further discussion.

@1ambda 1ambda mentioned this pull request Nov 24, 2016
Closed
@asfgit asfgit closed this in 5595bfb Nov 24, 2016
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
2 participants
@1ambda @bzz