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

Javanica iss 907 #945

Merged
merged 16 commits into from
Oct 21, 2015
Merged

Javanica iss 907 #945

merged 16 commits into from
Oct 21, 2015

Conversation

dmgcodevil
Copy link
Contributor

PL for hystrix javanica aspectj compile time weaving #907

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #212 FAILURE
Looks like there's a problem with this pull request

}
}

task ajcTest(type: Test) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattrjacobs I added this task that creates jar with aspects compiled using ajc, so now build task results to two jars:
hystrix-javanica.jar
hystrix-javanica-ctw.jar

Should we change something else to publish both artifacts correctly ?

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #213 FAILURE
Looks like there's a problem with this pull request

@mattrjacobs
Copy link
Contributor

Looks like there's a merge conflict on hystrix-javanica/build.gradle, according to: https://netflixoss.ci.cloudbees.com/job/NetflixOSS/job/Hystrix/job/Hystrix-pull-requests/212/console

Can you try to resolve that, so I can build locally and see what the gradle tasks do?

@dmgcodevil
Copy link
Contributor Author

@mattrjacobs yes, fill resolve.

…nica-iss-907

Conflicts:
	hystrix-contrib/hystrix-javanica/build.gradle
@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #218 SUCCESS
This pull request looks good

@dmgcodevil
Copy link
Contributor Author

@mattrjacobs I've got successful build https://netflixoss.ci.cloudbees.com/job/NetflixOSS/job/Hystrix/job/Hystrix-pull-requests/218/
could you check that we correctly release two artifacts: hystrix-javanica and hystrix-javanica-ctw ?

@mattrjacobs
Copy link
Contributor

Great, I'm checking it out now @dmgcodevil . Thanks for taking a look at all of the Javanica issues

@mattrjacobs
Copy link
Contributor

@dmgcodevil When I run the assemble task for hystrix-javanica, I get the following 2 artifacts:

hystrix-javanica-1.5.0-SNAPSHOT.jar
hystrix-javanica-ctw.jar

The ctw jar should have a version number, so that artifacts can get distinguished. Can you add that?

@dmgcodevil
Copy link
Contributor Author

@mattrjacobs Which plugin do you use to release artifact? How I can get version within gradle script ?
is it ok if ctw jar task will assemble jar once with artifact version ?

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #221 SUCCESS
This pull request looks good

@dmgcodevil
Copy link
Contributor Author

@mattrjacobs fixed, could you try again ?

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #222 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #223 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #224 SUCCESS
This pull request looks good

@mattrjacobs
Copy link
Contributor

@dmgcodevil This PR fixes the naming of the hystrix-javanica-ctw artifact and all unit tests pass. Is there any other verification of the compile-time weaving functionality that needs to happen before I merge this in?

@dmgcodevil
Copy link
Contributor Author

@mattrjacobs I've redesigned tests to check both compile and runtime weaving, also created simple app to check CTW and it works, I didn't find any issues. So go ahead to merge this PL

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #231 SUCCESS
This pull request looks good

mattrjacobs added a commit that referenced this pull request Oct 21, 2015
@mattrjacobs mattrjacobs merged commit 4dec35d into Netflix:master Oct 21, 2015
@mattrjacobs
Copy link
Contributor

Thanks @dmgcodevil !

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.

3 participants