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

✨ Make it possible to run envtest-based integration tests from vscode #8088

Conversation

dlipovetsky
Copy link
Contributor

What this PR does / why we need it:
Those of developing cluster-api using vscode need to run envtest-based integration tests. To do this, we must (a) set up envtest manually, and (b) configure vscode to discover the envtest artifacts. Every time the envtest version changes, we have to repeat these steps. These steps are not documented. Developers often resort to make test, and lose the features of vscode for running and debugging tests.

This PR creates (a) configures vscode-go to read environment variables from a file, and (b) creates a "vscode task" that runs a make target to set up envtest, and to record the envtest artifact location to the file.

Once the task is run, developers can run integration tests directly from vscode.

Developers can optionally enable the task to automatically run whenever they open the workspace.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 8, 2023
@dlipovetsky
Copy link
Contributor Author

/cc @Jont828

@dlipovetsky dlipovetsky force-pushed the dlipovetsky/vscode-envtest-integration branch from 8dd4dba to 4202349 Compare February 8, 2023 19:43
@dlipovetsky
Copy link
Contributor Author

(Sorry, I accidentally left some lines uncommitted; amended and force-pushed)

@Jont828
Copy link
Contributor

Jont828 commented Feb 9, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 4e400552e8d9197a80558f40016886b778418b0b

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2023
@dlipovetsky
Copy link
Contributor Author

I'll squash now, since the first commits are no longer relevant at all.

@dlipovetsky dlipovetsky force-pushed the dlipovetsky/vscode-envtest-integration branch from a2dc9a4 to e0f12f9 Compare February 11, 2023 00:30
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

Thanks @dlipovetsky!

@ykakarap I think you're using VScode? Any insight into how you're currently running envtest tests would be useful 🙂

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@sbueringer
Copy link
Member

Thx!
Looks pretty good to me.

Let's see if some other VSCode users want to chime in. (I'm using Intellij)

/lgtm

Feel free to ping me back for an approval in a bit :)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ea27f85112d9e06b5d9555118529836f9bfe6b24

@sbueringer
Copy link
Member

/cherry-pick release-1.3

@k8s-infra-cherrypick-robot

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.3 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dlipovetsky
Copy link
Contributor Author

Thanks for the reviews.

Come to think of it, any project that uses envtest (probably most infrastructure providers) can use this approach.

@richardcase
Copy link
Member

This worked a treat for me, this is great. Thanks @dlipovetsky 🙇

One comment, should we move the .vscode-example directory somewhere else? The reason I ask is that someone may want to do the same for another editor/ide and would we then end up with .idea_example and .myide_example in the root. I'm wondering if we have a developer directory somewhere else where we can include this and other examples of ide configurations?

@dlipovetsky
Copy link
Contributor Author

should we move the .vscode-example directory somewhere else?

Good question!

The reason I ask is that someone may want to do the same for another editor/ide and would we then end up with .idea_example and .myide_example in the root.

I can see how it might be bothersome, although personally I'm ok with it, since the directories are "hidden."

On the other hand, being hidden, the directories are had to discover for new users.

I'm wondering if we have a developer directory somewhere else where we can include this and other examples of ide configurations?

I don't see one, but I'd be open to this. For example

cluster-api/dev
├── cluster-api/dev/idea-example-configuration
└── cluster-api/dev/vscode-example-configuration

@killianmuldoon
Copy link
Contributor

I think it would be cleaner to have this in a text block in the CAPI book, so we don't need to keep example files around. This is what we do e.g. for the tilt-settings file, and it works well IMO.

@dlipovetsky
Copy link
Contributor Author

I think it would be cleaner to have this in a text block in the CAPI book, so we don't need to keep example files around. This is what we do e.g. for the tilt-settings file, and it works well IMO.

The user can apply tilt-settings by pasting the contents to one file. Here, the user must paste the contents to two files (settings.json and tasks.json). Pasting can be more error-prone than copying; for example, you can forget to create one of the two files, or make a typo naming one of the files.

I thought having the example files in the repo would make easy what I assume is the common use, where the user does not have .vscode/{settings,tasks}.json files.

But I'm fine with copy-paste, too. I'm just trying to help out fellow vscode users 😄

@Jont828
Copy link
Contributor

Jont828 commented Feb 17, 2023

I'd personally prefer to have a file where I can just rename to use the stock config. I like the idea of having everything packaged together like a dotfiles directory and that way you don't have to set up the folder structure too.

@Jont828
Copy link
Contributor

Jont828 commented Feb 22, 2023

/lgtm

@richardcase
Copy link
Member

For something like this with multiple files, I'd agree with @Jont828 that being to rename or copy a whole directory is easier than having to cut & paste from docs.

I like the idea of the dev folder like you suggest:

cluster-api/dev
├── cluster-api/dev/idea-example-configuration
└── cluster-api/dev/vscode-example-configuration

And then the docs can say something like:

cp -r cluster-api/dev/vscode-example-configuration .vscode

@killianmuldoon
Copy link
Contributor

I'm happy with the idea of a dev folder - I'm just sensitive to including config for external tools in the repo, but happy to give a try to see how it works 🙂

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2023
@killianmuldoon
Copy link
Contributor

Link check should be fine - it's failing as expected and will be fixed on merge.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ffcf7cdb032ae772b3400b16434fb41b350f9ffd

@dlipovetsky
Copy link
Contributor Author

(The PR Markdown links job fails, but the links will work once the PR is merged)

@dlipovetsky
Copy link
Contributor Author

dlipovetsky commented Feb 27, 2023

I'd like to squash the fixup commits, but that'll remove the lgtm...

Not sure if "squash merge" is an option?

@killianmuldoon
Copy link
Contributor

I'd like to squash the fixup commits, but that'll remove the lgtm...

You can squash the commits and I'll lgtm again 🙂 We don't do squash merge normally.

@sbueringer
Copy link
Member

@dlipovetsky If you only squash the commits without rebasing onto main the lgtm will stay. (the git tree hash stored in #8088 (comment) (details) has to be the same)

@dlipovetsky dlipovetsky force-pushed the dlipovetsky/vscode-envtest-integration branch from 9f6a669 to fc1f6d0 Compare February 27, 2023 18:35
@dlipovetsky
Copy link
Contributor Author

Thanks for the feedback, everyone! 🙏

@killianmuldoon
Copy link
Contributor

@dlipovetsky could you squash this down to a single commit?

* Add 'setup-envtest' make target to setup envtest on demand.
* Document how to configure VSCode.
@dlipovetsky dlipovetsky force-pushed the dlipovetsky/vscode-envtest-integration branch from fc1f6d0 to 4888af5 Compare March 1, 2023 17:06
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2023
@killianmuldoon
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 9311781ea1062cc55d68bf8ad441f99ee8fed246

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: killianmuldoon

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 1, 2023
@sbueringer sbueringer merged commit 9fe7ff5 into kubernetes-sigs:main Mar 1, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone Mar 1, 2023
@k8s-infra-cherrypick-robot

@sbueringer: new pull request created: #8212

In response to this:

/cherry-pick release-1.3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants