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

MP Config TCK - fix a regression caused by #2629 #2743

Merged
merged 3 commits into from Jun 11, 2019

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Jun 6, 2019

Could we also enable the tcks profile for CI?

@geoand
Copy link
Contributor

geoand commented Jun 6, 2019

Let me check this out locally and run it with the tck enabled.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGTM and worked like a charm!

@geoand
Copy link
Contributor

geoand commented Jun 6, 2019

I would say we should add the TCK tests to CI, but that's just me :)

@mkouba
Copy link
Contributor Author

mkouba commented Jun 6, 2019

@kenfinnigan @stuartwdouglas @cescoffier @gsmet Hey guys, who can modify the CI config? We just need to add -Dtcks.

@gsmet
Copy link
Member

gsmet commented Jun 6, 2019

@mkouba apparently, you found your answer :)

@cescoffier
Copy link
Member

Waiting for CI, we would need to check the output to verify the -Dtcks

@cescoffier cescoffier added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 6, 2019
@mkouba
Copy link
Contributor Author

mkouba commented Jun 6, 2019

@gsmet well, it was @geoand who told me where to look ;-)

@geoand
Copy link
Contributor

geoand commented Jun 6, 2019

@mkouba Looks like the -Dtcks was added to the wrong task. It should be added at https://github.com/quarkusio/quarkus/blob/master/azure-pipelines.yml#L75 instead of https://github.com/quarkusio/quarkus/blob/master/azure-pipelines.yml#L43

@mkouba
Copy link
Contributor Author

mkouba commented Jun 6, 2019

@geoand I don't think so. We don't want to run the tcks for Build_JDK11_Linux. Well, we could run it for this job too. But primarily for Build_Native_Linux.

@mkouba
Copy link
Contributor Author

mkouba commented Jun 6, 2019

The error is:

org.eclipse.aether.resolution.ArtifactResolutionException: Could not find artifact io.quarkus:quarkus-tck-microprofile-config:jar:999-SNAPSHOT

@geoand
Copy link
Contributor

geoand commented Jun 6, 2019

Why would we add it to the native job? I suppose it couldn't hurt, but that job is very slow as it is, so I would assume it would be best not to add it there.

Also in light of the error you mentioned what if we do the following:

Add a new job that first does mvn install -DskipTests -Dtcks (to install everything needed), then changes to the tck directory and executes them like mvn test -Dtcks.
That way we would know when a test failure is a TCK failure immediately.

@mkouba
Copy link
Contributor Author

mkouba commented Jun 6, 2019

Build_Native_Linux is basically all-in-one JDK8 job. +1 for a new TCK-only job. But I'm not sure if that's acceptable. Would you care to communicate this?

@geoand
Copy link
Contributor

geoand commented Jun 6, 2019

Sure thing.

@geoand
Copy link
Contributor

geoand commented Jun 8, 2019

@cescoffier do you think we could add a new job as in CI as part of this? Do you see any reason why not to do it?

@cescoffier
Copy link
Member

@geoand sure, we can add more CI jobs.
That should be doable in the yaml file (in the project root).

@cescoffier cescoffier removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 11, 2019
@geoand
Copy link
Contributor

geoand commented Jun 11, 2019

Great! @mkouba would you like to try that approach and see how it goes?

@mkouba
Copy link
Contributor Author

mkouba commented Jun 11, 2019

@geoand This PR should be open for changes. It would be great if you could take this over ;-).

@geoand
Copy link
Contributor

geoand commented Jun 11, 2019

@mkouba sure thing, I will take care of it later on today :)

@geoand geoand force-pushed the mp-config-tck-fix branch 4 times, most recently from c19d8f5 to d172d14 Compare June 11, 2019 10:46
@geoand
Copy link
Contributor

geoand commented Jun 11, 2019

TCKs are working, I'll merge as soon as the rest of the jobs go green

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 11, 2019
@mkouba
Copy link
Contributor Author

mkouba commented Jun 11, 2019

@geoand Looks good! Thanks.

@geoand
Copy link
Contributor

geoand commented Jun 11, 2019

No problem!

@geoand geoand merged commit ca52194 into quarkusio:master Jun 11, 2019
@mkouba mkouba removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 13, 2019
@gsmet gsmet added this to the 0.17.0 milestone Jun 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants