alefragnani / vscode-jenkins-status

Jenkins Status Extension for Visual Studio Code
MIT License
27 stars 20 forks source link

support running in remote extension host #76

Closed kalinkrustev closed 2 years ago

kalinkrustev commented 2 years ago

Use the same trick, as in eslint plugin here: https://github.com/microsoft/vscode-eslint/blob/f71e3089a0e63c9b4e94f1ea4264f90599f5ddab/server/src/eslintServer.ts#L502

alefragnani commented 2 years ago

Hi @kalinkrustev ,

Thanks for the PR, but I'm not sure what the effect of this change. And in fact, that's not what #41 is about.

The idea behind #41 is to enable the extension to work on remotes like SSH, WSL, Container, without the need to install the extension on that remote. To accomplish this, a few changes (API and contributing related) needs to be made.

Could you please provide more details about the meaning of this change?

Thank you

kalinkrustev commented 2 years ago

OK, I am not sure how easy it is to achieve that without installing the extension at the remote. My proposal makes it work when the extension is installed at the remote. The effect of this change is to use the original require, not the webpack one, to read the contents from .jenkinsrc.js on the remote. Without this change, it tries to read it from the webpacked extension, where it obviously does not exist.

alefragnani commented 2 years ago

The .jenkinsrc.js file is not bundled with the extension. This is an alternative to the .jenkins file, if you need add some script to provide the URLs, like discovering the current branch to be sent to the Jenkins request. This was created in this PR a few years ago https://github.com/alefragnani/vscode-jenkins-status/pull/17#issuecomment-587417349

If that's not your scenario, there will be no need to install the extension on the remote (once it is updated to work with remotes). The extension should be installed only locally, and it would work correctly on any remote (Container, WSL, SSH, Codespaces).

I mean, if your .jenkinsrc.js file is something like this:

// can also return a promise of required JSON structure
module.exports = [{
    "url": "http://127.0.0.1:8080/job/myproject/",
    "name": "Jenkins Build",
    "username": "jenkins_user",
    "password": "jenkins_password_or_token"
},
{
    "url": "http://127.0.0.1:8080/job/myprojectTests/",
    "name": "Jenkins Acceptance Tests",
    "username": "jenkins_user",
    "password": "jenkins_password_or_token"
}];

You could easily change to the .jenkins format

[
    {
        "url": "http://127.0.0.1:8080/job/myproject/",
        "name": "Jenkins Build",
        "username": "jenkins_user",
        "password": "jenkins_password_or_token"
    },
    {
        "url": "http://127.0.0.1:8080/job/myprojectTests/",
        "name": "Jenkins Acceptance Tests",
        "username": "jenkins_user",
        "password": "jenkins_password_or_token"
    }
]

And it has the same effect.

kalinkrustev commented 2 years ago

I need it to be .jenkinsrc.js, because I need to put some logic and and not use static URLs. The approach of installing the extension on the remote is just an easier way to achieve the functionality. So this change just makes sure that when using require() to load .jenkinsrc.js it is not trying to load the file from the webpacked output (which is what is currently happening). The eslint extension (also installed in the remote) from which I borrowed the solution is doing basically the same thing, when it tries to load its config files.

alefragnani commented 2 years ago

That's really weird the extension to have issues loading .jenkinsrc.js inside on remote, even when installed on remote. It should work just like a local installation. But I could confirm that it does not work. 😕

Using your snippet, it indeed does work, and I'll apply it to this scenario. But in order to keep the extension easier to use (installed locally and working on remotes), other changes are needed (the regular support remotes approach). This will support the default .jenkins file, locally and remotely.

Thank you

alefragnani commented 2 years ago

Closing, in favor of #78