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

feat(sdk): inline user defined custom tasks #636

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

ScrapCodes
Copy link
Contributor

@ScrapCodes ScrapCodes commented Jun 24, 2021

Which issue is resolved by this Pull Request:
Resolves #627

Description of your changes:
inline user defined custom tasks
Added tests, docs.

Environment tested:

  • Python Version (use python --version):
  • Tekton Version (use tkn version):
  • Kubernetes Version (use kubectl version):
  • OS (e.g. from /etc/os-release):

Checklist:

@ScrapCodes
Copy link
Contributor Author

ScrapCodes commented Jun 24, 2021

To be merged after #633

Thank you @Tomcli, for such a detailed guide to implement this issue. 😄

- **--taskSpec** (optional): Kubernetes Resource Spec for your custom task CRD. The value needs to define in Python Dictionary.
- **--taskSpec** (optional): Kubernetes Resource Spec for your custom task CRD. One of `--taskSpec` or `--taskRef` can be specified at a time.
The value should be a Python Dictionary.
- **--taskRef** (optional): Kubernetes Resource Spec for your custom task CRD. This gets inlined in the pipeline. One of `--taskSpec` or `--taskRef` can be specified at a time.
Copy link
Member

Choose a reason for hiding this comment

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

taskSpec should be the one that inlined in the pipeline. taskRef just generate the spec in a separate CRD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I was thinking, if we swap the taskSpec meaning, the custom controller who do not yet support embedded spec, will stop working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@pugangxa
Copy link
Contributor

I saw the file changed in PR #633 also exists in this PR, I think the main purpose is the document right? The document change looks good to me.

@ScrapCodes
Copy link
Contributor Author

ScrapCodes commented Jun 29, 2021

Hi @pugangxa, main change is 98d2f90

[EDIT] updated after doing a self pass of review a9dcaaa

Now that it is rebased, the entire PR can be reviewed.

@Tomcli
Copy link
Member

Tomcli commented Jun 30, 2021

@ScrapCodes need to rebase this

@ScrapCodes
Copy link
Contributor Author

@Tomcli rebased !!

# See the License for the specific language governing permissions and
# limitations under the License.

apiVersion: custom_task_api_version
Copy link
Member

Choose a reason for hiding this comment

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

Just quick question, isn't custom_task_ref.py will generate this CRD since we are using taskRef?

Copy link
Contributor Author

@ScrapCodes ScrapCodes Jul 1, 2021

Choose a reason for hiding this comment

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

It is generated in tmp/ dir and cleaned up. Our tests do not assert the generated CRDs. Do you think it should be changed?

Copy link
Member

Choose a reason for hiding this comment

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

I opened an issue on it, but we didn't get time to implement it yet.
#610

@ScrapCodes ScrapCodes force-pushed the todo2 branch 4 times, most recently from 9368157 to 1d7e1fd Compare July 1, 2021 14:08
@Tomcli
Copy link
Member

Tomcli commented Jul 2, 2021

/lgtm
/approve

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ScrapCodes, Tomcli

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

@google-oss-robot google-oss-robot merged commit dbd92cb into kubeflow:master Jul 2, 2021
@ScrapCodes ScrapCodes deleted the todo2 branch September 3, 2021 09:05
ScrapCodes added a commit to ScrapCodes/kfp-tekton that referenced this pull request Oct 18, 2021
google-oss-robot pushed a commit that referenced this pull request Oct 20, 2021
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.

Update SDK to support embedded spec for custom task
4 participants