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

update scala #253

Merged
merged 7 commits into from Oct 3, 2018
Merged

update scala #253

merged 7 commits into from Oct 3, 2018

Conversation

davidmargolin
Copy link
Contributor

Updates scala to 2.12.7
resolves #251

@CLAassistant
Copy link

CLAassistant commented Oct 3, 2018

CLA assistant check
All committers have signed the CLA.

@pauljamescleary
Copy link
Contributor

Thanks @davidmargolin!! Also, feel free to add yourself to our AUTHORS.md if you feel so inclined.

Letting travis to its thing :)

@davidmargolin
Copy link
Contributor Author

looks like more than i thought... switching to WIP

@davidmargolin davidmargolin changed the title update scala [WIP] update scala Oct 3, 2018
@pauljamescleary
Copy link
Contributor

@davidmargolin we haven't linked to profiles yet. If you add yourself to AUTHORS, you can do a link to your github profile if you wish. I may add mine as well. It really is an individual choice.

@pauljamescleary
Copy link
Contributor

Interesting on the build failure, one of them succeeded and the other failed? I may just rebuild that one, perhaps some travis cache woes?

@nimaeskandary
Copy link
Contributor

May even need to clear out the travis caches on a scala update, unsure

@rebstar6
Copy link
Contributor

rebstar6 commented Oct 3, 2018

@davidmargolin try updating the version in .travis.yml too

@codecov
Copy link

codecov bot commented Oct 3, 2018

Codecov Report

Merging #253 into master will increase coverage by 0.01%.
The diff coverage is 22.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #253      +/-   ##
==========================================
+ Coverage   95.12%   95.14%   +0.01%     
==========================================
  Files         123      123              
  Lines        4859     4858       -1     
  Branches      106      105       -1     
==========================================
  Hits         4622     4622              
+ Misses        237      236       -1
Impacted Files Coverage Δ
...in/scala/vinyldns/api/route/RecordSetRouting.scala 98.55% <ø> (ø) ⬆️
.../scala/vinyldns/api/route/VinylDNSDirectives.scala 60% <0%> (+1.93%) ⬆️
...n/scala/vinyldns/api/route/MembershipRouting.scala 100% <100%> (ø) ⬆️
...rc/main/scala/vinyldns/api/route/ZoneRouting.scala 98.59% <100%> (ø) ⬆️

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 d1d9d4c...3bff38b. Read the comment docs.

@pauljamescleary
Copy link
Contributor

It maybe possible also that the new scala version is finding a few unused things in our code

  • MembershipRoute - line 96 - path("groups" / Segment) { id =>, id is not used, can change this to _
  • RecordSetRoute - line 107 (zoneId, rsId, changeId) => the rsId is not used, this can change to _
  • VinylDNSDirectives - line 39 - I don't think we need the extractExecutionContext any more, so that can be deleted
  • ZoneRoute - line 79 - (put & path("zones" / Segment) & monitor("Endpoint.updateZone")) { id =>, the id at the end there is not used, can be changed to _

Build Output

[error] /home/travis/build/vinyldns/vinyldns/modules/api/src/main/scala/vinyldns/api/route/MembershipRouting.scala:96:34: parameter value id in value $anonfun is never used
[error]       path("groups" / Segment) { id =>
[error]                                  ^
[error] /home/travis/build/vinyldns/vinyldns/modules/api/src/main/scala/vinyldns/api/route/RecordSetRouting.scala:107:18: parameter value rsId in value $anonfun is never used
[error]         (zoneId, rsId, changeId) =>
[error]                  ^
[error] /home/travis/build/vinyldns/vinyldns/modules/api/src/main/scala/vinyldns/api/route/VinylDNSDirectives.scala:39:48: parameter value ec in value $anonfun is never used
[error]     extractExecutionContext.flatMap { implicit ec ⇒
[error]                                                ^
[error] /home/travis/build/vinyldns/vinyldns/modules/api/src/main/scala/vinyldns/api/route/ZoneRouting.scala:79:74: parameter value id in value $anonfun is never used
[error]       (put & path("zones" / Segment) & monitor("Endpoint.updateZone")) { id =>
[error]                                                                          ^
[error] four errors found

@pauljamescleary
Copy link
Contributor

Confirmed, same thing happens locally. The adjustments I mentioned above worked.

@davidmargolin willing to make those changes ⬆️ in my last comment and post an update to the PR. No unit tests or anything else need to be made. Let me know if you have any questions.

@pauljamescleary
Copy link
Contributor

I fixed the initial ones locally, and there are sooo many more. Apologies @davidmargolin thought this would be simpler. Squashing the compiler warnings is annoying

@pauljamescleary
Copy link
Contributor

@davidmargolin feel free to hop in the gitter channel if you need any assistance. I think there are maybe about a dozen compiler warnings that need to be fixed as part of this upgrade. We will be happy to help out with anything.

Copy link
Contributor

@pauljamescleary pauljamescleary left a comment

Choose a reason for hiding this comment

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

Just one comment, I think everything else is good

@davidmargolin
Copy link
Contributor Author

davidmargolin commented Oct 3, 2018

finally haha. Thanks for the opportunity and the help
edit: oof spoke too soon

@davidmargolin davidmargolin changed the title [WIP] update scala update scala Oct 3, 2018
Copy link
Contributor

@pauljamescleary pauljamescleary left a comment

Choose a reason for hiding this comment

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

🎉

Thanks a lot for your help @davidmargolin!!!

Copy link
Contributor

@rebstar6 rebstar6 left a comment

Choose a reason for hiding this comment

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

ran this locally, looks good!

@pauljamescleary pauljamescleary merged commit 0161676 into vinyldns:master Oct 3, 2018
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.

Upgrade to scala 2.12.7
5 participants