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

Support on prem GitHub release notes / changelog #1563

Merged

Conversation

eugeniyk
Copy link
Contributor

@eugeniyk eugeniyk commented Aug 6, 2020

Currently we have github.com and other urls hardcoded in the code, which makes it unusable in environments with different on-prem domain names.

Fixes #1088

@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #1563 into master will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1563      +/-   ##
==========================================
+ Coverage   70.63%   70.77%   +0.13%     
==========================================
  Files         113      113              
  Lines        1822     1827       +5     
  Branches       53       51       -2     
==========================================
+ Hits         1287     1293       +6     
+ Misses        535      534       -1     
Impacted Files Coverage Δ
.../scala/org/scalasteward/core/vcs/VCSExtraAlg.scala 100.00% <ø> (ø)
...main/scala/org/scalasteward/core/vcs/package.scala 100.00% <100.00%> (ø)
...g/scalasteward/core/application/SupportedVCS.scala 53.84% <0.00%> (+7.69%) ⬆️

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 6525ca6...68393ed. Read the comment docs.

@mzuehlke
Copy link
Member

mzuehlke commented Aug 6, 2020

Your change would restrict linking to release notes to only the VCS the current installation is running on.
In the current implementation the scala steward instance running on Gitlab or even on-prem could link to release notes on e.g. github.com. After your change this would not be possible anymore.

Should this PR address: #1088 ? Just to understand your motivation.

@eugeniyk
Copy link
Contributor Author

eugeniyk commented Aug 7, 2020

@mzuehlke I've got your point, I thought it's trying to compare within current github installation.
currently our github installation uses "github.domain.io", making it's impossible to link CHANGELOG for local dependencies

Seems #1088 describes exactly my problem

@eugeniyk
Copy link
Contributor Author

eugeniyk commented Aug 8, 2020

@mzuehlke looks like we can solve it this way:

  1. if repoUri starts with Config.vcsApiHost then we can use Config.vcsType and apply current PR changes
  2. otherwise fallback to the current implementation

WDYT?

@mzuehlke
Copy link
Member

mzuehlke commented Aug 9, 2020

I think your proposal looks like a perfect way to solve this issue. 👍

@eugeniyk
Copy link
Contributor Author

eugeniyk commented Aug 9, 2020

image

Looks like false alarm for me

@eugeniyk
Copy link
Contributor Author

eugeniyk commented Aug 9, 2020

@mzuehlke please review again

@eugeniyk eugeniyk changed the title Support on prem GitHub release notes Support on prem GitHub release notes / changelog Aug 11, 2020
@eugeniyk
Copy link
Contributor Author

@fthomas could you please review this PR? The Codacy failure sounds like false positive

@mzuehlke mzuehlke self-requested a review September 3, 2020 14:51
Copy link
Member

@mzuehlke mzuehlke left a comment

Choose a reason for hiding this comment

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

A really good PR.
Let me know what you think about my comments.
But I'm fine with merging this PR like it is now.

@@ -73,51 +73,90 @@ package object vcs {
possibleFilenames(baseNames)
}

def possibleCompareUrls(repoUrl: Uri, update: Update): List[VersionDiff] = {
private[this] def extractRepoVCSType(
Copy link
Member

Choose a reason for hiding this comment

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

one more, related to the first comment:
extractRepoVCSType could live in VCSExtraAlg and you would only need to pass the RepoVCSType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then possibleCompareUrls and possibleReleaseRelatedUrls would need to be extracted to VCSExtraAlg as well (right now it's in package object), which make sense for me as well

I can try to make it as separated PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mzuehlke have it as separated PR here: eugeniyk#1
I think we can merge #1563 as is and continue with refactorings in PR above, wdyt?

@mzuehlke mzuehlke merged commit c74f7f7 into scala-steward-org:master Sep 25, 2020
@mzuehlke
Copy link
Member

Thanks @eugeniyk for contributing to scala-steward 👍

@mzuehlke mzuehlke added the enhancement New feature or request label Sep 25, 2020
@eugeniyk
Copy link
Contributor Author

@mzuehlke glad to improve this awesome tool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compare url & release notes/changelog are restricted to {github,gitlab}.com
2 participants