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

Added Location header to run creation #701

Merged
merged 1 commit into from
Jan 16, 2020

Conversation

nkijak
Copy link
Contributor

@nkijak nkijak commented Jan 14, 2020

Fix for #699

@nkijak nkijak force-pushed the feature/run-location-header branch 2 times, most recently from a646f57 to b1d3352 Compare January 14, 2020 14:15
@codecov
Copy link

codecov bot commented Jan 14, 2020

Codecov Report

Merging #701 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #701     +/-   ##
===========================================
+ Coverage     65.78%   65.89%   +0.1%     
  Complexity      339      339             
===========================================
  Files           109      109             
  Lines          1888     1894      +6     
  Branches        136      136             
===========================================
+ Hits           1242     1248      +6     
  Misses          401      401             
  Partials        245      245
Impacted Files Coverage Δ Complexity Δ
src/main/java/marquez/api/JobResource.java 54.76% <100%> (+3.47%) 12 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb1a936...7884ffb. Read the comment docs.

@nkijak
Copy link
Contributor Author

nkijak commented Jan 14, 2020

First time using JAX-RS, so not sure if this is the best way to get the location header. Didn't want to hardcode the value but the reflection step to get the method seems like it might be bad.

@wslulciuc wslulciuc added the review Ready for review label Jan 14, 2020
@@ -33,6 +33,10 @@
@NonNull Run.State state;
@Nullable Map<String, String> args;

public UUID getId() {
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Lombok will generate getters/setters for us. So, we don't need to explicitly define one.

Copy link
Member

@wslulciuc wslulciuc left a comment

Choose a reason for hiding this comment

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

@nkijak For your first time using JAX-RS, this PRs solid. Great work! 💯 Sure, there may be alternative ways to add the Location header, but I'm ok with merging this for now.

Note: Left a minor comment on using lombok to generate mutators for Run. I'll merge once addressed.

@nkijak
Copy link
Contributor Author

nkijak commented Jan 15, 2020

@wslulciuc made the update but the commits are messy. Should I close this and open another?

@wslulciuc
Copy link
Member

@nkijak My PRs get messy as well. I recommend squashing your commits into one. Then, we can merge this sweet feature!

@nkijak nkijak force-pushed the feature/run-location-header branch from c9a2281 to 7f1745b Compare January 16, 2020 02:58
Fix for MarquezProject#699

Signed-off-by: Nicolas Kijak <nick.kijak@pngaming.com>
@nkijak nkijak force-pushed the feature/run-location-header branch from 7f1745b to 7884ffb Compare January 16, 2020 03:02
@nkijak
Copy link
Contributor Author

nkijak commented Jan 16, 2020

@wslulciuc squashed

@wslulciuc wslulciuc merged commit c979f9a into MarquezProject:master Jan 16, 2020
@wslulciuc wslulciuc removed the review Ready for review label Jan 16, 2020
@nkijak nkijak deleted the feature/run-location-header branch January 16, 2020 20:15
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.

2 participants