Skip to content

Centralize harded code message#3409

Closed
fahedouch wants to merge 3 commits intocontainerd:masterfrom
fahedouch:centralize-harded-code-messag
Closed

Centralize harded code message#3409
fahedouch wants to merge 3 commits intocontainerd:masterfrom
fahedouch:centralize-harded-code-messag

Conversation

@fahedouch
Copy link
Copy Markdown
Member

@fahedouch fahedouch commented Jul 14, 2019 •
edited by crosbymichael
Loading

Hello,

Centralize harded code message.

closes: #3349

fahedouch added 3 commits July 13, 2019 15:07
@fahedouch
centralize harded-code message
db95af4 
Signed-off-by: Fahed Dorgaa <[email protected]>
@fahedouch
fix variable name
70b00a0 
Signed-off-by: Fahed Dorgaa <[email protected]>
@fahedouch
fix name in containers file
abc152d 
Signed-off-by: Fahed Dorgaa <[email protected]>
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jul 14, 2019

Build succeeded.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 14, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3409      +/-   ##
==========================================
- Coverage   48.98%   45.05%   -3.94%     
==========================================
  Files         102      113      +11     
  Lines        9903    12563    +2660     
==========================================
+ Hits         4851     5660     +809     
- Misses       4207     6049    +1842     
- Partials      845      854       +9
Flag Coverage Δ
#linux 48.98% <ø> (ø) ⬆️
#windows 40.27% <ø> (?)
Impacted Files Coverage Δ
errdefs/errors.go 100% <ø> (ø) ⬆️
snapshots/native/native.go 43.04% <0%> (-9.99%) ⬇️
cio/io.go 46.47% <0%> (-8.53%) ⬇️
metadata/snapshot.go 47.52% <0%> (-8.26%) ⬇️
archive/tar.go 43.79% <0%> (-7.07%) ⬇️
metadata/containers.go 47.97% <0%> (-6.62%) ⬇️
content/local/writer.go 58.65% <0%> (-5.55%) ⬇️
content/local/store.go 49.52% <0%> (-5.29%) ⬇️
metadata/images.go 57.57% <0%> (-4.99%) ⬇️
archive/tar_opts.go 28.57% <0%> (-4.77%) ⬇️
... and 64 more

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 f2b6c31...abc152d. Read the comment docs.

estesp
estesp approved these changes Jul 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

dmcgowan
dmcgowan requested changes Jul 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

Same feedback from closed PR

#3391 (comment)

stevvooe
stevvooe requested changes Jul 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

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

Not LGTM

These are unique errors. Centralizing them makes them really hard to track down. Centralizing errors should only be used when API has caller dependencies.

@stevvooe stevvooe closed this Jul 15, 2019
@crosbymichael crosbymichael mentioned this pull request Jul 15, 2019
Merged
@fahedouch
Copy link
Copy Markdown
Member Author

fahedouch commented Jul 15, 2019

Not LGTM

These are unique errors. Centralizing them makes them really hard to track down. Centralizing errors should only be used when API has caller dependencies.

I undrestand @stevvooe thanks for the explanation.

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

Reviewers

@stevvooe stevvooe stevvooe requested changes

@dmcgowan dmcgowan dmcgowan requested changes

@estesp estesp estesp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

centralize harded-code message

5 participants

@fahedouch @codecov-io @stevvooe @dmcgowan @estesp