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: show warning msg when sanitize dev #3021

Merged
merged 10 commits into from
Aug 24, 2022

Conversation

AdrianPedriza
Copy link
Contributor

@AdrianPedriza AdrianPedriza commented Aug 19, 2022

Fixes #2888

Proposed changes

  • sanitize dev section

Signed-off-by: adripedriza <adripedriza@gmail.com>
Signed-off-by: adripedriza <adripedriza@gmail.com>
pkg/model/manifest.go Outdated Show resolved Hide resolved
pkg/model/manifest.go Outdated Show resolved Hide resolved
@AdrianPedriza AdrianPedriza changed the title Adrian/show warning msg when sanitize dev fix: show warning msg when sanitize dev Aug 19, 2022
@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

Merging #3021 (5ba9a1b) into master (df13ed4) will increase coverage by 0.01%.
The diff coverage is 61.70%.

@@            Coverage Diff             @@
##           master    #3021      +/-   ##
==========================================
+ Coverage   32.72%   32.73%   +0.01%     
==========================================
  Files         188      188              
  Lines       19743    19773      +30     
==========================================
+ Hits         6460     6473      +13     
- Misses      12519    12532      +13     
- Partials      764      768       +4     
Impacted Files Coverage Δ
cmd/up/up.go 10.71% <ø> (ø)
pkg/cmd/stack/deploy.go 25.86% <0.00%> (ø)
pkg/model/manifest.go 26.10% <48.27%> (+0.76%) ⬆️
pkg/model/serializer.go 67.52% <88.23%> (-0.14%) ⬇️

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

Signed-off-by: adripedriza <adripedriza@gmail.com>
pkg/model/manifest.go Outdated Show resolved Hide resolved
Signed-off-by: adripedriza <adripedriza@gmail.com>
@AdrianPedriza
Copy link
Contributor Author

the main problem with the needed solution is to find a proper way to show the user all the sanitized warnings in a friendly manner.

Current display:

Captura de pantalla 2022-08-22 a las 9 27 54

@AdrianPedriza
Copy link
Contributor Author

the main problem with the needed solution is to find a proper way to show the user all the sanitized warnings in a friendly manner.

Current display:

Captura de pantalla 2022-08-22 a las 9 27 54

after discussing with @jLopezbarb about what should be the proper way to show the user this type of warning, as a temporary solution before the manifest parsing refactor we agree to display the user two kinds of warnings: one for okteto manifest and another one for compose files.

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

@AdrianPedriza Could you post a screenshot of the final result?

@AdrianPedriza
Copy link
Contributor Author

AdrianPedriza commented Aug 22, 2022

@AdrianPedriza Could you post a screenshot of the final result?

sure.

for okteto up

Captura de pantalla 2022-08-22 a las 16 25 42

for okteto deploy

Captura de pantalla 2022-08-22 a las 16 30 41

@jLopezbarb
Copy link
Contributor

@AdrianPedriza Could you post a screenshot of the final result?

sure.

for okteto up

Captura de pantalla 2022-08-22 a las 16 25 42

for okteto deploy

Captura de pantalla 2022-08-22 a las 16 26 16

I'd mention in the okteto deploy that the second message is referencing a compose file, but other than that looks good to me

@AdrianPedriza
Copy link
Contributor Author

@AdrianPedriza Could you post a screenshot of the final result?

sure.
for okteto up
Captura de pantalla 2022-08-22 a las 16 25 42
for okteto deploy
Captura de pantalla 2022-08-22 a las 16 26 16

I'd mention in the okteto deploy that the second message is referencing a compose file, but other than that looks good to me

yes, sorry it was an old pic. Check it now, I have edited the pic!

pkg/cmd/stack/deploy.go Outdated Show resolved Hide resolved
Signed-off-by: adripedriza <adripedriza@gmail.com>
pkg/cmd/stack/deploy.go Outdated Show resolved Hide resolved
pkg/model/manifest.go Outdated Show resolved Hide resolved
pkg/model/manifest.go Show resolved Hide resolved
Signed-off-by: adripedriza <adripedriza@gmail.com>
pkg/model/manifest.go Outdated Show resolved Hide resolved
Signed-off-by: adripedriza <adripedriza@gmail.com>
@derek
Copy link

derek bot commented Aug 24, 2022

Thank you for your contribution. unfortunately, one or more of your commits are missing the required "Signed-off-by:" statement. Signing off is part of the Developer Certificate of Origin (DCO) which is used by this project.

Read the DCO and project contributing guide carefully, and amend your commits using the git CLI. Note that this does not require any cryptography, keys or special steps to be taken.

💡 Shall we fix this?

This will only take a few moments.

First, clone your fork and checkout this branch using the git CLI.

Next, set up your real name and email address:

git config --global user.name "Your Full Name"
git config --global user.email "you@domain.com"

Finally, run one of these commands to add the "Signed-off-by" line to your commits.

If you only have one commit so far then run: git commit --amend --signoff and then git push --force.
If you have multiple commits, watch this video.

Check that the message has been added properly by running "git log".

@derek derek bot added the no-dco label Aug 24, 2022
Signed-off-by: adripedriza <adripedriza@gmail.com>
@AdrianPedriza AdrianPedriza force-pushed the adrian/show-warning-msg-when-sanitize-dev branch from 77e85a8 to 5ba9a1b Compare August 24, 2022 08:47
@jLopezbarb
Copy link
Contributor

I forgot to mention that this PR needs to update docs

@AdrianPedriza
Copy link
Contributor Author

I forgot to mention that this PR needs to update docs

where?

@jLopezbarb
Copy link
Contributor

I forgot to mention that this PR needs to update docs

where?

https://github.com/okteto/docs

@AdrianPedriza
Copy link
Contributor Author

I forgot to mention that this PR needs to update docs

where?

https://github.com/okteto/docs

I mean, in which parts of docs we could talk about this kind of warnings?

@AdrianPedriza
Copy link
Contributor Author

I forgot to mention that this PR needs to update docs

where?

https://github.com/okteto/docs

I mean, in which parts of docs we could talk about this kind of warnings?

I know what you mean know. I have created this issue due to at the moment we need to wait for other PR to be integrated into docs.

@AdrianPedriza AdrianPedriza merged commit 677f454 into master Aug 24, 2022
@AdrianPedriza AdrianPedriza deleted the adrian/show-warning-msg-when-sanitize-dev branch August 24, 2022 10:52
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.

okteto up doesn't work properly when the service name contains _ and the deploy replaced that char by -
4 participants