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

[SPARK-26882] Check the Kubernetes integration tests scalatyle #23792

Conversation

holdenk
Copy link
Contributor

@holdenk holdenk commented Feb 14, 2019

What changes were proposed in this pull request?

Add the kubernetes integration tests to the scalastyle profiles.

How was this patch tested?

Run ./dev/scalastyle with a bad change manually

Follow on work

See SPARK-26898 to add scalastyle for k8s integration to the CI

@holdenk
Copy link
Contributor Author

holdenk commented Feb 14, 2019

The WIP part is to resolve any scala style issues that pop up during CI.

@SparkQA
Copy link

SparkQA commented Feb 14, 2019

@SparkQA
Copy link

SparkQA commented Feb 14, 2019

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/7955/

@SparkQA
Copy link

SparkQA commented Feb 15, 2019

Test build #102365 has finished for PR 23792 at commit a159aff.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@@ -29,6 +29,7 @@ ERRORS=$(echo -e "q\n" \
-Phive-thriftserver \
-Pspark-ganglia-lgpl \
-Pdocker-integration-tests \
-Pkubernetes-integration-tests \
Copy link
Member

Choose a reason for hiding this comment

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

I recall the way this is setup right now, buildin the profile will try to execute the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean we aren't building the profile per-se, we're only evaluating scalastyle test:scalastyle on it, but let me double check locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running ./dev/scalastyle after this change locally does not trigger the k8s integration tests

@vanzin
Copy link
Contributor

vanzin commented Feb 15, 2019

Hmm, I wonder how style errors were not triggered here? Maybe the PR builders don't use that script?

I had to make a style change in Minikube.scala as part of #23793, because sbt complains about it.

@holdenk
Copy link
Contributor Author

holdenk commented Feb 15, 2019

@vanzin I think the PR builders do a different style check yeah, I'll dig into that as well and keep this as WIP.

@holdenk
Copy link
Contributor Author

holdenk commented Feb 15, 2019

@vanzin Looking at Jenkins it looks like we depend on the scala-style-during-compile configuration now days for CI and the k8s integration tests have a skip flag turned on for that.

@SparkQA
Copy link

SparkQA commented Feb 15, 2019

@SparkQA
Copy link

SparkQA commented Feb 15, 2019

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/7982/

@holdenk
Copy link
Contributor Author

holdenk commented Feb 16, 2019

Ok I think fixing this in CI is probably better done in a separate PR, I've created a follow up issue SPARK-26898 , I think someone who is more familiar with how the K8s integration build is set up will make more progress on that.

@holdenk holdenk marked this pull request as ready for review February 16, 2019 00:52
@holdenk holdenk changed the title [WIP][SPARK-26882] Check the Kubernetes integration tests scalatyle [SPARK-26882] Check the Kubernetes integration tests scalatyle Feb 16, 2019
@SparkQA
Copy link

SparkQA commented Feb 16, 2019

Test build #102402 has finished for PR 23792 at commit c50f10d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin
Copy link
Contributor

vanzin commented Feb 19, 2019

Merging to master. Although I really wish some build would fail if there was a style issue, preferably a PR build.

@vanzin vanzin closed this in 6b3c832 Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants