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

fix context checking when no manifest found #3008

Merged
merged 13 commits into from
Aug 18, 2022

Conversation

AdrianPedriza
Copy link
Contributor

Signed-off-by: adripedriza adripedriza@gmail.com

Fixes #2977

Proposed changes

  • fix error to check to discovery.ErrOktetoManifestNotFound
  • initialize context when err == discovery.ErrOktetoManifestNotFound

Signed-off-by: adripedriza <adripedriza@gmail.com>
@codecov
Copy link

codecov bot commented Aug 16, 2022

Codecov Report

Merging #3008 (83a8474) into master (df13ed4) will increase coverage by 0.01%.
The diff coverage is 30.00%.

@@            Coverage Diff             @@
##           master    #3008      +/-   ##
==========================================
+ Coverage   32.72%   32.73%   +0.01%     
==========================================
  Files         188      188              
  Lines       19743    19748       +5     
==========================================
+ Hits         6460     6465       +5     
+ Misses      12519    12518       -1     
- Partials      764      765       +1     
Impacted Files Coverage Δ
cmd/manifest/init-v2.go 0.00% <0.00%> (ø)
cmd/up/up.go 10.71% <0.00%> (ø)
pkg/model/manifest.go 25.34% <20.00%> (ø)
cmd/context/utils.go 22.22% <50.00%> (+3.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jLopezbarb
Copy link
Contributor

Could we unify oktetoErrors.ErrManifestNotFound and discovery.ErrManifestNotFound? I think it can be error-prone to have both

@ifbyol
Copy link
Member

ifbyol commented Aug 16, 2022

Could we unify oktetoErrors.ErrManifestNotFound and discovery.ErrManifestNotFound? I think it can be error-prone to have both

Agree with unifying both errors if they are used to represent the same. Also, careful with the error handling if we do the change

Signed-off-by: adripedriza <adripedriza@gmail.com>
Signed-off-by: adripedriza <adripedriza@gmail.com>
cmd/context/utils.go Outdated Show resolved Hide resolved
Signed-off-by: adripedriza <adripedriza@gmail.com>
Signed-off-by: adripedriza <adripedriza@gmail.com>
Signed-off-by: adripedriza <adripedriza@gmail.com>
Signed-off-by: adripedriza <adripedriza@gmail.com>
@ifbyol
Copy link
Member

ifbyol commented Aug 17, 2022

Can we run the E2E before merging it?

Signed-off-by: adripedriza <adripedriza@gmail.com>
Signed-off-by: adripedriza <adripedriza@gmail.com>
@teresaromero
Copy link
Member

i remember going thought this with #2969 can we check this case is covered with this change?

Signed-off-by: adripedriza <adripedriza@gmail.com>
cmd/context/utils.go Outdated Show resolved Hide resolved
cmd/manifest/init-v2.go Outdated Show resolved Hide resolved
Signed-off-by: adripedriza <adripedriza@gmail.com>
Signed-off-by: adripedriza <adripedriza@gmail.com>
Signed-off-by: adripedriza <adripedriza@gmail.com>
@AdrianPedriza AdrianPedriza merged commit 3a59d8c into master Aug 18, 2022
@AdrianPedriza AdrianPedriza deleted the adrian/improve-context-checking-when-no-manifest branch August 18, 2022 12:51
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.

Improve the way we get the context when checking if a manifest exists
4 participants