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 support for openshift build config #305

Merged
merged 1 commit into from
Dec 5, 2020

Conversation

HarikrishnanBalagopal
Copy link
Contributor

When targeting Openshift, generate a BuildConfig if Tekton
is not available for CI/CD purposes.

Signed-off-by: Harikrishnan Balagopal harikrishmenon@gmail.com

@codecov
Copy link

codecov bot commented Dec 4, 2020

Codecov Report

Merging #305 (9a128bb) into master (73b70af) will decrease coverage by 0.53%.
The diff coverage is 1.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
- Coverage   24.81%   24.28%   -0.54%     
==========================================
  Files          70       71       +1     
  Lines        4429     4522      +93     
==========================================
- Hits         1099     1098       -1     
- Misses       3226     3320      +94     
  Partials      104      104              
Impacted Files Coverage Δ
internal/apiresource/apiresource.go 0.00% <ø> (ø)
internal/apiresource/buildconfig.go 0.00% <0.00%> (ø)
internal/apiresource/deployment.go 0.00% <0.00%> (ø)
internal/apiresource/eventlistener.go 0.00% <ø> (ø)
internal/apiresource/imagestream.go 0.00% <0.00%> (ø)
internal/apiresource/knativeservice.go 0.00% <ø> (ø)
internal/apiresource/networkpolicy.go 92.30% <ø> (ø)
internal/apiresource/pipeline.go 0.00% <0.00%> (ø)
internal/apiresource/role.go 0.00% <ø> (ø)
internal/apiresource/rolebinding.go 0.00% <ø> (ø)
... and 9 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 73b70af...9a128bb. Read the comment docs.

Copy link
Member

@ashokponkumar ashokponkumar left a comment

Choose a reason for hiding this comment

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

Nice work Hari. Overall it looks great.

Few comments:

Do we have conditions to ensure that if a Openshift cluster (has buildconfig) supports tekton we create only tekton? I did not see any such condition in buildconfig createnewresource.

Also, do should we create buildconfig and imagestream even if the cluster supports only deployment (means somebody has manually disabled deployment config). This is a corner case, we can handle it as part of sensible defaults (we can create a new issue for it).

case strings.Contains(gitRepoURL, "bitbucket"):
return okdbuildv1.BitbucketWebHookBuildTriggerType
default:
log.Warnf("the git repo is not on github, gitlab or bitbucket. Defaulting to generic web hook trigger")
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this to be a debug message? Since in many cases the we might not have found a git repo url

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have conditions to ensure that if a Openshift cluster (has buildconfig) supports tekton we create only tekton? I did not see any such condition in buildconfig createnewresource.
https://github.com/HarikrishnanBalagopal/move2kube/blob/buildconfig/internal/transformer/cicdtransformer.go#L39-L50

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, do should we create buildconfig and imagestream even if the cluster supports only deployment (means somebody has manually disabled deployment config). This is a corner case, we can handle it as part of sensible defaults (we can create a new issue for it).

To get k8s deployments to use image streams we have to add certain annotations https://developers.redhat.com/blog/2019/09/20/using-red-hat-openshift-image-streams-with-kubernetes-deployments/

  annotations:
    deployment.kubernetes.io/revision: "2"
    image.openshift.io/triggers: '[{"from":{"kind":"ImageStreamTag","name":"ubi:8"},"fieldPath":"spec.template.spec.containers[?(@.name==\"myapp\")].image"}]'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want this to be a debug message? Since in many cases the we might not have found a git repo url

If there's no repo should we just avoid creating CICD resources?

Copy link
Member

Choose a reason for hiding this comment

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

We can create it if there is request from customers. For now, create an issue with this link

Copy link
Member

Choose a reason for hiding this comment

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

Do we want this to be a debug message? Since in many cases the we might not have found a git repo url

If there's no repo should we just avoid creating CICD resources?

Let it be there, with placeholder for git repo.

ashokponkumar
ashokponkumar previously approved these changes Dec 5, 2020
ashokponkumar
ashokponkumar previously approved these changes Dec 5, 2020
ashokponkumar
ashokponkumar previously approved these changes Dec 5, 2020
When targeting Openshift, generate a BuildConfig if Tekton
is not available for CI/CD purposes.

Signed-off-by: Harikrishnan Balagopal <harikrishmenon@gmail.com>
@ashokponkumar ashokponkumar merged commit d2b0fc1 into konveyor:master Dec 5, 2020
@HarikrishnanBalagopal HarikrishnanBalagopal deleted the buildconfig branch December 5, 2020 13:49
@HarikrishnanBalagopal HarikrishnanBalagopal added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
2 participants