Skip to content

Conversation

@kerinin
Copy link
Contributor

@kerinin kerinin commented May 16, 2019

No description provided.

@codecov-io
Copy link

Codecov Report

Merging #39 into master will decrease coverage by 2.24%.
The diff coverage is 9.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
- Coverage   52.91%   50.66%   -2.25%     
==========================================
  Files          13       13              
  Lines         567      598      +31     
==========================================
+ Hits          300      303       +3     
- Misses        218      245      +27     
- Partials       49       50       +1
Impacted Files Coverage Δ
cmd/cli/operations/retrieve_savepoint.go 41.17% <9.67%> (-48.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 706b264...a1c98e2. Read the comment docs.

Copy link
Contributor

@hustclf hustclf left a comment

Choose a reason for hiding this comment

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

Hi, you should not commit files under vendor

@kerinin
Copy link
Contributor Author

kerinin commented May 17, 2019

I'm happy to remove them, but there are lots of files committed under vendor in master, I assumed you guys were vendoring dependencies?

@hustclf
Copy link
Contributor

hustclf commented May 18, 2019

I am not sure why it happens before either, is it necessary to add a .gitignore to remove vendor in master ? @mrooding

If it does, I would like to do it.

@mrooding
Copy link

Thanks for the contribution @kerinin.

We do actually commit the vendor directory. As stated in the dep FAQ (https://github.com/golang/dep/blob/master/docs/FAQ.md#should-i-commit-my-vendor-directory), it does have a con of polluting the PR but it also has some nice advantages.

I'll get to reviewing this beginning of next week!

@joshuavijay
Copy link
Contributor

@mrooding, is there anyway you could review and and get this PR merged asap? I need this functionality. Thanks @kerinin

u, err := url.Parse(dir)

if err == nil && s3Schemes[u.Scheme] {
return o.retrieveLatestSavepointS3(u)
Copy link
Contributor

Choose a reason for hiding this comment

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

There could be multiple jobs running in the same flink cluster. Do you assume the savepoint dir param to this function is unique to each job?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants