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

Move typescript code to ts #514

Merged
merged 3 commits into from
Feb 26, 2022
Merged

Conversation

yuwenma
Copy link
Contributor

@yuwenma yuwenma commented Feb 24, 2022

  • Move ./go (mainly contains typescript related code) to ./ts
  • keep ./go/api (this dir includes Kptfile schema)
  • Move ./script (mainly for typescript) to ./ts
  • Delete tests/e2e
  • Simplify github cicd
  • complete cleanup #331

@yuwenma
Copy link
Contributor Author

yuwenma commented Feb 25, 2022

@mengqiy This is ready to review. I removed two required presubmit checks: the e2e tests as we discussed offline and the go-ci which checks ./go.

Some other tasks I plan to do in separate PRs

  • completed remove ./ts/go. I saw you have some previous work about the go cleanup. cleanup #331 +100 to remove out of date code.
  • disable the node-ci since we do not have plan to modify typescript in short term. We can add it back easily if removing the ci in a separate PR.
  • Maybe add a new ci for ./api when we plan to update the kptfile.

@mengqiy
Copy link
Contributor

mengqiy commented Feb 25, 2022

Move ./go/api to ./api (this dir includes Kptfile schema)

IIUC the plan is to use this repo to host TS and golang SDK. The stuff under go/api is not defined in a IDL, it is the golang language binding for Kptfile. What if we have the TS language binding for Kptfile in the future? Where should it live?
So IMO go/api is a reasonable location.

@mengqiy
Copy link
Contributor

mengqiy commented Feb 25, 2022

We should revive #331. I don't have bandwidth for it. Can you please pick it up?

@mengqiy
Copy link
Contributor

mengqiy commented Feb 25, 2022

disable the node-ci

I don't see it. Did I miss anything?

@yuwenma
Copy link
Contributor Author

yuwenma commented Feb 25, 2022

We should revive #331. I don't have bandwidth for it. Can you please pick it up?

For sure.

@yuwenma
Copy link
Contributor Author

yuwenma commented Feb 25, 2022

disable the node-ci

I don't see it. Did I miss anything?

That is the change "I plan to do in separate PRs". Just want to double check if you have any concerns of removing that from the CI?

@yuwenma
Copy link
Contributor Author

yuwenma commented Feb 25, 2022

Move ./go/api to ./api (this dir includes Kptfile schema)

IIUC the plan is to use this repo to host TS and golang SDK. The stuff under go/api is not defined in a IDL, it is the golang language binding for Kptfile. What if we have the TS language binding for Kptfile in the future? Where should it live? So IMO go/api is a reasonable location.

TS, golang and Starlark. kustomize plans to remove starlark support. So we should consider our own starlark support to make it meet the bar for our SDK users.

moved to go/api

@mengqiy
Copy link
Contributor

mengqiy commented Feb 26, 2022

That is the change "I plan to do in separate PRs". Just want to double check if you have any concerns of removing that from the CI?

I see.

disable the node-ci since we do not have plan to modify typescript in short term.

This doesn't sound like a good reason to disable the node CI. Why not just leave it there?

gosdk/go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise LGTM

@yuwenma
Copy link
Contributor Author

yuwenma commented Feb 26, 2022

That is the change "I plan to do in separate PRs". Just want to double check if you have any concerns of removing that from the CI?

I see.

disable the node-ci since we do not have plan to modify typescript in short term.

This doesn't sound like a good reason to disable the node CI. Why not just leave it there?

yeah, after renaming those typescript workflow, agree it's better to leave it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants