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

Add gas check to make lint, fix unnecessary assignment #1015

Merged
merged 3 commits into from
Oct 26, 2016
Merged

Conversation

riyazdf
Copy link
Contributor

@riyazdf riyazdf commented Oct 21, 2016

Removes the parent directory creation code from client.go because it's already handled by the underlying cache.

Also adds github.com/HewlettPackard/gas to the make lint check, though it's a little bit hacky until securego/gosec#55 and securego/gosec#50 are resolved.

@@ -777,8 +777,7 @@ func (r *NotaryRepository) saveMetadata(ignoreSnapshot bool) error {
}

for role, blob := range targetsToSave {
parentDir := filepath.Dir(role)
os.MkdirAll(parentDir, 0755)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for removing this!

@@ -777,8 +777,7 @@ func (r *NotaryRepository) saveMetadata(ignoreSnapshot bool) error {
}

for role, blob := range targetsToSave {
parentDir := filepath.Dir(role)
os.MkdirAll(parentDir, 0755)
// If the parent directory does not exist, the cache.Set will create it
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this code comment is suitable for all the other metadata store implementations like memorystore and httpstore, they may need not to "create" the parent directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it seems it will always init as FilesystemStore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HuKeping: yup this function is only used for clients saving local copies of the repo metadata to files on disk

@riyazdf riyazdf changed the title [WIP] add gas check to make lint, fix unnecessary assignment Add gas check to make lint, fix unnecessary assignment Oct 24, 2016
@HuKeping
Copy link
Contributor

Tested it locally and it works great! Have rebased to the master. LGTM!

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Copy link
Contributor

@cyli cyli left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks for tackling this and adding it to lint! LGTM

@cyli cyli merged commit 3ca7b72 into master Oct 26, 2016
@cyli cyli deleted the gas-check branch October 26, 2016 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants