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

Add pid file creation for okteto up command. #860

Merged
merged 4 commits into from
May 5, 2020

Conversation

adhaamehab
Copy link
Contributor

Signed-off-by: adhaamehab adhaamehab.me@gmail.com

Fixes #691

Proposed changes

  • Okteto Up generates a namespace-deployment.pid file at .okteto/ folder defined in the $OKTETOHOME
  • The pid file gets deleted once okteto up finishes
  • If the namespace isn't defined in okteto.yml file or --namespace flag. we use the namespace provided by the k8s client

@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #860 into master will decrease coverage by 0.00%.
The diff coverage is 38.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #860      +/-   ##
==========================================
- Coverage   33.14%   33.13%   -0.01%     
==========================================
  Files          67       67              
  Lines        5253     5275      +22     
==========================================
+ Hits         1741     1748       +7     
- Misses       3369     3381      +12     
- Partials      143      146       +3     
Impacted Files Coverage Δ
cmd/up.go 6.23% <38.88%> (+1.10%) ⬆️

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 4e32f8a...ecbfa09. Read the comment docs.

Copy link
Member

@rberrelleza rberrelleza left a comment

Choose a reason for hiding this comment

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

Looks good! Minor comments added.

cmd/up.go Outdated Show resolved Hide resolved
cmd/up.go Outdated Show resolved Hide resolved
cmd/up.go Outdated Show resolved Hide resolved
cmd/up.go Outdated Show resolved Hide resolved
cmd/up.go Outdated
if err != nil {
return err
}
dev.Namespace = namespace
Copy link
Contributor

@pchico83 pchico83 May 4, 2020

Choose a reason for hiding this comment

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

The namespace value might come from the namespace flag too.
dev.Namespace is initialized at this point:

}

The RunUp logic is a bit messy, we need to refactor it. But you could bring the dev.Namespace initialization logic here if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, @pchico83 I'm a little bit confused because the referenced line is older than the current. Can you explain more?

Copy link
Contributor

@pchico83 pchico83 May 4, 2020

Choose a reason for hiding this comment

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

Sure. In the master branch, dev.Namespace is initialized in lines 220-230 in cmd/up.go.
I think you can move that logic here to have a single place where where k8Client.GetLocal() is called:
Screenshot 2020-05-04 at 21 45 24

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks.
Will do so 👍🏻

Signed-off-by: adhaamehab <adhaamehab.me@gmail.com>
@adhaamehab
Copy link
Contributor Author

@rberrelleza @pchico83
Can you review the latest changes whenever you have time?

cmd/up.go Outdated Show resolved Hide resolved
@derek derek bot added the no-dco label May 5, 2020
@derek
Copy link

derek bot commented May 5, 2020

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

Signed-off-by: adhaamehab <adhaamehab.me@gmail.com>
cmd/up.go Outdated Show resolved Hide resolved
Signed-off-by: adhaamehab <adhaamehab.me@gmail.com>
@rberrelleza
Copy link
Member

Thanks for adding the test, looks great!

@derek
Copy link

derek bot commented May 5, 2020

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

@derek derek bot added the no-dco label May 5, 2020
@rberrelleza rberrelleza merged commit cc079a7 into okteto:master May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

generate a pid file
3 participants