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

Support --redeploy=false to deploy a pipeline only if the repo is not… #1437

Merged
merged 2 commits into from
Apr 27, 2021

Conversation

pchico83
Copy link
Contributor

… already deployed

Signed-off-by: Pablo Chico de Guzman pchico83@gmail.com

This makes it possible to define cyclic dependencies between pipelines

@codecov
Copy link

codecov bot commented Apr 21, 2021

Codecov Report

Merging #1437 (13ff089) into master (2602d4f) will decrease coverage by 0.13%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1437      +/-   ##
==========================================
- Coverage   36.58%   36.44%   -0.14%     
==========================================
  Files          79       79              
  Lines        7292     7320      +28     
==========================================
  Hits         2668     2668              
- Misses       4293     4321      +28     
  Partials      331      331              
Impacted Files Coverage Δ
cmd/pipeline/deploy.go 15.74% <0.00%> (-1.21%) ⬇️
pkg/okteto/pipeline.go 0.00% <0.00%> (ø)

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 2602d4f...13ff089. Read the comment docs.

@rberrelleza
Copy link
Member

rberrelleza commented Apr 21, 2021

What do you mean by cyclic dependencies @pchico83 ? Could you explain the use case this enables a bit more?

@rberrelleza
Copy link
Member

rberrelleza commented Apr 21, 2021

I think it's odd that --redeploy=true is the default, given that the whole purpose of the command is to deploy the pipeline.

Have we considered instead having a flag to not run the pipeline if already exists? --skip-if-exists cc @rlamana

@rlamana
Copy link
Contributor

rlamana commented Apr 22, 2021

@rberrelleza @pchico83 I don't think it is odd for default behavior, but I do like --skip-if-exists. Searching for it I actually found many CLIs using a flag with the same name, so I guess that the rule of the "most common use" wins here.

@pchico83
Copy link
Contributor Author

@rlamana @rberrelleza I will switch to --skip-if-exists

@pchico83
Copy link
Contributor Author

What do you mean by cyclic dependencies @pchico83 ? Could you explain the use case this enables a bit more?

@rberrelleza when you have pipelines that call other pipelines, it is sometimes easier to define the pipeline with cyclic references, especially when deploying preview environments. For example, let's say you have 5 repositories, and there is a PR on repo A.
It is easy to say deploy the pipeline of repo A from current branch, and then, call a metapipeline that deploys the 5 repos. If the metapipeline deploys repo A with --skip-if-exists everything is fine.
We should think about a more declarative definition for this, but this unblocks several scenarios for now.

@derek
Copy link

derek bot commented Apr 23, 2021

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 Apr 23, 2021
@derek derek bot removed the no-dco label Apr 23, 2021
… already deployed

Signed-off-by: Pablo Chico de Guzman <pchico83@gmail.com>
Signed-off-by: Pablo Chico de Guzman <pchico83@gmail.com>
@rberrelleza
Copy link
Member

What do you mean by cyclic dependencies @pchico83 ? Could you explain the use case this enables a bit more?

@rberrelleza when you have pipelines that call other pipelines, it is sometimes easier to define the pipeline with cyclic references, especially when deploying preview environments. For example, let's say you have 5 repositories, and there is a PR on repo A.
It is easy to say deploy the pipeline of repo A from current branch, and then, call a metapipeline that deploys the 5 repos. If the metapipeline deploys repo A with --skip-if-exists everything is fine.
We should think about a more declarative definition for this, but this unblocks several scenarios for now.

The part I don't get is why do you need skip-if-exists for that scenario? Don't we want all the dependencies to be redeployed together?

@pchico83
Copy link
Contributor Author

What do you mean by cyclic dependencies @pchico83 ? Could you explain the use case this enables a bit more?

@rberrelleza when you have pipelines that call other pipelines, it is sometimes easier to define the pipeline with cyclic references, especially when deploying preview environments. For example, let's say you have 5 repositories, and there is a PR on repo A.
It is easy to say deploy the pipeline of repo A from current branch, and then, call a metapipeline that deploys the 5 repos. If the metapipeline deploys repo A with --skip-if-exists everything is fine.
We should think about a more declarative definition for this, but this unblocks several scenarios for now.

The part I don't get is why do you need skip-if-exists for that scenario? Don't we want all the dependencies to be redeployed together?

@rberrelleza it makes the preview pipeline logic harder. For example, you deploy repo A in branch test, and then, if you deploy the metapipeline, the metapipeline will redeploy repo A in branch master. This is not the case if the metapipeline does okteto pipeline deploy -r repoA --if-not-exists
Another setup is when you have repo A and B. Pipeline A deploys A and B (if it doesn't exist), and pipeline B deploys B and A (if it doesn't exist). Then, deploying A or B will deploys all your needed components, without entering cyclic loops.
All this should be handled in more declarative way by application templates, but for now, it unblocks several scenarios that customers are asking for

@pchico83 pchico83 merged commit f7258e2 into master Apr 27, 2021
@pchico83 pchico83 deleted the pchico83/pipeline-redeploy branch April 27, 2021 08:33
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.

None yet

4 participants