Removing ./pkg/repo #316
Closed
Removing ./pkg/repo #316
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
in favor of a function that does the same thing
hopefully this is easier to read
the ref returned from fetch does it
calls to fetch will fail if vgo is not installed
and using the new module go get fetcher
This was referenced Jul 20, 2018
This removes code that is no longer used and depends on the ./pkg/repo package
marpio
requested changes
Jul 20, 2018
| if err != nil { | ||
| return err | ||
| } | ||
| ret, err := ref.Read() |
| func (s *ModuleSuite) TestNewGoGetFetcher() { | ||
| r := s.Require() | ||
| fs := s.fs | ||
| fetcher, err := NewGoGetFetcher(fs, repoURI, version) |
There was a problem hiding this comment.
NewGoGetFetcher func has only one arg.
| // we need to use an OS filesystem because fetch executes vgo on the command line, which | ||
| // always writes to the filesystem | ||
| fs := afero.NewOsFs() | ||
| fetcher, err := NewGoGetFetcher(fs, repoURI, version) |
There was a problem hiding this comment.
NewGoGetFetcher func has only one arg.
| // TODO: return a ref for cleaning up the goPathRoot | ||
| return nil, err | ||
| } | ||
| sourcePath := filepath.Join(goPathRoot, "src", goPathRoot) |
There was a problem hiding this comment.
Shouldn't this be sourcePath := filepath.Join(goPathRoot, "src") ?
| goGetFetcher, ok := fetcher.(*goGetFetcher) | ||
| r.True(ok) | ||
| r.Equal(repoURI, goGetFetcher.repoURI) | ||
| r.Equal(version, goGetFetcher.version) |
There was a problem hiding this comment.
fields repoURI and version do not exists
marwan-at-work
requested changes
Jul 21, 2018
|
|
||
| // save it | ||
| if err := s.Save(context.Background(), module, version, modBytes, zipFile, infoBytes); err != nil { | ||
| if err := s.Save( | ||
| context.Background(), |
There was a problem hiding this comment.
i'd put a contextWithTimeout if there's no context being passed from above.
|
|
||
| func (s *ModuleSuite) TestNewGoGetFetcher() { | ||
| r := s.Require() | ||
| fs := s.fs |
There was a problem hiding this comment.
no need to assign a new variable here, just use s.fs once below.
marpio
reviewed
Jul 22, 2018
| _, err = e.Append(eventlog.Event{ | ||
| Module: modName, | ||
| Version: version, | ||
| Time: time.Now(), |
ghost
added
on hold
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.
This is a follow-up to #291 that removes the
./pkg/repopackage and replaces calls to it with the new./pkg/modulefunctionality that #291 added. #314 requires this.Requires #291 before merging this. Requires #291. Check out more details on why this is opened against #291 at #290 (comment)
This is part of #290
The only addition on top of #291 is 244014e. Refer to #291 for all other changes