-
Notifications
You must be signed in to change notification settings - Fork 131
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
Tool 1347 log new step version info #692
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
lszucs
commented
Jan 21, 2020
- isUpdateAvailable was moved to a separate file and heavily refactored, because of the new requirements
- update info printing got refactored in a separate function
- monkey patched SetInfoModel in vendor to make CI pass, until dependency is updated
trapacska
requested changes
Jan 21, 2020
bitrise/print.go
Outdated
content = updateRow | ||
if stepInfo.Step.SourceCodeURL != nil && *stepInfo.Step.SourceCodeURL != "" { | ||
content += "\n" + getRow("") | ||
releasesURL := *stepInfo.Step.SourceCodeURL + "/releases" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- pls check nil for ptr
- is it sure that adding /releases will always do the job we want? for example: https://github.com/bitrise-io/bitrise-steplib/blob/d3581bdf1de00ddbbe0f0c7113f75d855312e8b1/steps/codified-security-bitrise/1.0.1/step.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- bitbucket urls may has no /releases ending
- not sure why but I found occurrence or source code url not specified in the step.yml. is it more safe to get the source -> git field?
trapacska
approved these changes
Jan 21, 2020
trapacska
added a commit
that referenced
this pull request
Jan 21, 2020
* refactor: extract function * print resolved version when applicable * refactor: simplify conditionals * refactor: simplify conditional * test cases: version row for major and minor lock * build correct version info row string * refactor: remove useless code * print dotted changelog URL * refactor: put major and minor test case together * test case: url cropping * empty line in box under version info * dont show update info if latest version fits lock * remove development related temp code * remove unrelated logic from tests * improve test case name * refactor: simplify string production * test case for 1.x and 1.x.x format * refactor: extract update handling to separate file * refactor: consolidate update check flow * handle semver parse error * remove testing code * refactor: make naming consistent * monkey patch dependency * fallback to repo url when releases url is unknown * dep update Co-authored-by: lszucs <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.