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

lsbackup command to list backups. #3219

Merged
merged 8 commits into from
Apr 4, 2019
Merged

lsbackup command to list backups. #3219

merged 8 commits into from
Apr 4, 2019

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Mar 28, 2019

This change is Reviewable

@martinmr martinmr requested a review from a team March 29, 2019 21:24
@martinmr martinmr marked this pull request as ready for review March 29, 2019 21:24
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

LGTM. Get someone from the team to approve.

Reviewed 2 of 7 files at r1, 4 of 5 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @martinmr)


ee/backup/s3_handler.go, line 297 at r1 (raw file):

		}
	}
	if len(manifests) == 0 {

The code from here onwards, looks the same as the code in files. You can refactor this common code out, so the main caller function would do this logic, and use the handlers just to retrieve the manifests.

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 7 files reviewed, 2 unresolved discussions (waiting on @manishrjain and @martinmr)


ee/backup/s3_handler.go, line 297 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

The code from here onwards, looks the same as the code in files. You can refactor this common code out, so the main caller function would do this logic, and use the handlers just to retrieve the manifests.

Done.

@martinmr martinmr requested a review from a team April 2, 2019 20:59
@martinmr martinmr dismissed manishrjain’s stale review April 2, 2019 20:59

Addressed comments.

Copy link
Contributor

@codexnull codexnull left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 7 files at r1, 1 of 5 files at r2, 4 of 4 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @manishrjain and @martinmr)


ee/backup/backup.go, line 94 at r4 (raw file):

	FileName string
}

Did you consider embedding the Manifest struct? I.e.

type ManifestFile struct {
    Manifest
    FileName  string
}

Then you can do manifest.FileName and manifest.Version instead of manifest.Manifest.Version.


ee/backup/file_handler.go, line 180 at r4 (raw file):

func (h *fileHandler) ReadManifest(path string, m *Manifest) error {
	return h.readManifest(path, m)

Is there a reason for not exporting readManifest() instead?


ee/backup/run.go, line 126 at r4 (raw file):

Source URI formats:
  [scheme]://[host]/[path]?[args]
  [scheme]:///[path]?[args]

Can the scheme with a '///' URI be anything other than "file"?

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @codexnull, @golangcibot, and @manishrjain)


ee/backup/backup.go, line 94 at r4 (raw file):

Previously, codexnull (Javier Alvarado) wrote…

Did you consider embedding the Manifest struct? I.e.

type ManifestFile struct {
    Manifest
    FileName  string
}

Then you can do manifest.FileName and manifest.Version instead of manifest.Manifest.Version.

Done. I didn't know you could do that.


ee/backup/file_handler.go, line 180 at r4 (raw file):

Previously, codexnull (Javier Alvarado) wrote…

Is there a reason for not exporting readManifest() instead?

readManifest takes different arguments in the s3 handler.


ee/backup/handler.go, line 172 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not goimports-ed (from goimports)

Done.


ee/backup/run.go, line 126 at r4 (raw file):

Previously, codexnull (Javier Alvarado) wrote…

Can the scheme with a '///' URI be anything other than "file"?

Not so far but that might change in the future.

ee/backup/handler.go Outdated Show resolved Hide resolved
@martinmr martinmr merged commit af954fb into master Apr 4, 2019
@martinmr martinmr deleted the martinmr/lsbackup branch April 4, 2019 21:30
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants