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 tools dependency #4801

Merged
merged 3 commits into from
Jul 30, 2019
Merged

Update tools dependency #4801

merged 3 commits into from
Jul 30, 2019

Conversation

AdityaSripal
Copy link
Member

@AdityaSripal AdityaSripal commented Jul 29, 2019

Commit that referred to tools dependency does not exist in git history because of force push. Update go.mod to refer to latest commit

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: clog add [section] [-t <tag>] [-m <msg>]
  • Re-reviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@AdityaSripal AdityaSripal changed the title Remove tools dependency Update tools dependency Jul 29, 2019
@AdityaSripal AdityaSripal requested a review from mircea-c July 29, 2019 22:23
mircea-c
mircea-c previously approved these changes Jul 29, 2019
Copy link

@mircea-c mircea-c left a comment

Choose a reason for hiding this comment

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

LGTM.

Technically runsim is not a dependency of the project, but it is easier to have it in go.mod rather than having to ensure it never gets committed by anyone by mistake.

@codecov
Copy link

codecov bot commented Jul 29, 2019

Codecov Report

Merging #4801 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4801      +/-   ##
==========================================
- Coverage    50.5%   50.49%   -0.02%     
==========================================
  Files         288      288              
  Lines       18518    18518              
==========================================
- Hits         9353     9351       -2     
- Misses       8480     8482       +2     
  Partials      685      685

go.sum Outdated Show resolved Hide resolved
@AdityaSripal
Copy link
Member Author

agree with @colin-axner . since these files will change after go mod tidy, i think it makes sense to just have sdk developers adopt workflow so that they always run go mod tidy before a merge

Otherwise, we will have commits that either add or remove meaningless dependencies to SDK

Thoughts? @mircea-c

@AdityaSripal AdityaSripal added S:blocked Status: Blocked Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. labels Jul 29, 2019
@AdityaSripal AdityaSripal removed the S:blocked Status: Blocked label Jul 29, 2019
@AdityaSripal AdityaSripal requested a review from mircea-c July 29, 2019 23:25
@AdityaSripal AdityaSripal dismissed mircea-c’s stale review July 29, 2019 23:26

ran go mod tidy

@mircea-c mircea-c force-pushed the aditya/mod-fix branch 2 times, most recently from 255c26f to eb6b527 Compare July 30, 2019 00:20
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK

@alexanderbez alexanderbez merged commit 4d5d2f7 into master Jul 30, 2019
@mircea-c mircea-c deleted the aditya/mod-fix branch July 30, 2019 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants