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

Remove usage of Tekton resources #770

Conversation

SaschaSchwarze0
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 commented May 6, 2021

Changes

Related to Remove usage of Tekton resources #696.

Reviewers can go through the changes by commit:

  1. Extend deployment configuration to include template for Git step extends the deployment configuration with a template for a Git step. I chose JSON as serialization format as this is easier to provide as a one-liner in a deployment yaml. Once we switch to a ConfigMap, we can revisit how we store the data there. The image URL can be configured in the template but is also available as separate environment variable. That's needed as our release build sets only the image URL based on the git image that we build (in commit 5)
  2. Add custom gomega matcher adds a Gomega matcher to check for an element in an array or slice to contain an item with a certain name, is used in the unit tests in the next commit.
  3. Remove Git and Image resources, introduce common handling of sources, use results is the main code change. The resources are removed, results are added. I introduced a structure for our different source types so that as we evolve that, we have a nice structure in our source code. Remote artifacts were moved to sources/http to fit into that structure. The runtime image code was extended to correctly fill the image results in the same way as the Kaniko sample strategy (see commit 6).
  4. Stop adding source secret to the service account stops adding the source secret to the service account as this is directly mounted by the previous commit.
  5. Build git image as part of the release build extends our deployment with the environment variable for the git image in ko-style. In the release build, I had to workaround the ko limitation that only one image name can be specified (see comment in the file). I just notice that the release.sh here references the GIT_IMAGE environment variable, this is set in the Makefile in commit 7. @qu1queee please do not forget to create the git repository in quay as discussed, so that the image can be pushed by the build.
  6. Update sample build strategies to write to results updates the sample build strategies to write the image sha and size where possible. The EP also suggests solutions for BuildKit and maybe Buildah, those strategies can be extended in a follow-on PR. I focused on those that already supported that feature plus Buildpacks where it is trivial to access the image SHA. The strategy changes also outline that less chown in prepare steps is necessary for non-root build strategies. Remaining is the chown for the Tekton home directory that we will get rid of once we also move away from the service account and creds-init for the output image credential.
  7. Update integration test makes the necessary changes to the integration test cases and updates our CI to build the git image and load it into the KinD cluster.
  8. Update documentation adds the two results that the build strategy author can use to the build strategy documentation. The authentication documentation is updated to remove the Tekton annotation as that is not needed anymore. I am also removing the link to Tekton's authentication documentation. There's nothing for our users to learn there and we evolve to our own secret handling anyway.
  9. Update EP with information on how the non-root git clone is implemented updates the EP with information about how to the non-root Git clone is done, and adjusts some naming for the volume mount to prevent possible conflicts with what build strategy authors could use by using the shp- prefix there.

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

Release Notes

Remove usage of Tekton's pipelines resources in favor of a shipwright-managed step to clone a Git repository, and Tekton results to store information such as Git commit SHA and image digest and size

@openshift-ci-robot openshift-ci-robot added the release-note Label for when a PR has specified a release note label May 6, 2021
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from saschaschwarze0 after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@SaschaSchwarze0 SaschaSchwarze0 force-pushed the sascha-696-remove-resources branch 2 times, most recently from c121440 to 06aa6c9 Compare May 6, 2021 14:32
@SaschaSchwarze0 SaschaSchwarze0 added the kind/feature Categorizes issue or PR as related to a new feature. label May 7, 2021
@SaschaSchwarze0 SaschaSchwarze0 changed the title WIP Remove usage of Tekton resources May 7, 2021
Copy link
Contributor

@HeavyWombat HeavyWombat left a comment

Choose a reason for hiding this comment

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

I have not finished the full PR yet. We were able to check on the CI parts, publishing and integration tests. However, there are already things I would like to start a discussion.

pkg/config/config.go Show resolved Hide resolved
pkg/config/config_test.go Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
hack/release.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

Adding some comments on my first review. I still need to understand the override on the previous implementation @otaviof had for spec.sources, and then all changes when generating the Task/TaskRun, plus strategy renames. So pls expect 2 more reviews.

docs/development/authentication.md Outdated Show resolved Hide resolved
pkg/config/config.go Show resolved Hide resolved
pkg/config/config.go Show resolved Hide resolved
Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

Next round of request for changes. Looks good in general, nice work!

pkg/reconciler/buildrun/resources/sources/git.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/sources/git.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/taskrun.go Show resolved Hide resolved
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2021
Copy link
Contributor

@HeavyWombat HeavyWombat left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2021
@qu1queee
Copy link
Contributor

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qu1queee

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2021
@openshift-merge-robot openshift-merge-robot merged commit e3f2d05 into shipwright-io:main May 18, 2021
@SaschaSchwarze0 SaschaSchwarze0 deleted the sascha-696-remove-resources branch May 18, 2021 07:13
@adambkaplan adambkaplan added this to the release-v0.5.0 milestone Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Label for when a PR has specified a release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants