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

update dependencies; migrate shellescape package to new import path #113

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

pietdevries94
Copy link
Contributor

A little while back the import path of github.com/alessio/shellescape changed to al.essio.dev/pkg/shellescape . This PR handles that, so people can update again.

Signed-off-by: Piet de Vries <piet@devries.tech>
@mikkeloscar mikkeloscar added the dependencies Pull requests that update a dependency file label Oct 16, 2024
@mikkeloscar
Copy link
Member

👍

@RomanZavodskikh
Copy link
Member

👍

@mikkeloscar mikkeloscar merged commit 4e6ecc1 into zalando:master Oct 25, 2024
8 checks passed
@andyfeller
Copy link

andyfeller commented Oct 31, 2024

A little while back the import path of github.com/alessio/shellescape changed to al.essio.dev/pkg/shellescape . This PR handles that, so people can update again.

Could you explain the benefits and tradeoffs involved with source Go modules from GitHub Pages?

I understand alessio/shellescape#26 changed this, however my knee jerk is holding off upgrading zalando/go-keyring until the security implications of dependencies hosted on GitHub Pages can be assessed.

@mikkeloscar
Copy link
Member

I think this is best raised to the upstream as we have no influence how the upstream module is defined.

My understanding is that without this change we block updating zalando/go-keyring via Go modules. If that is not the case, then this is maybe not needed. I agree that it's questionable in terms of security and reliability to not rely on github.com paths when the module code is anyway on github.com.

@szuecs
Copy link
Member

szuecs commented Nov 1, 2024

@mikkeloscar you can also inline the code. The repository is about 3 functions in total so a little dependency is worse than a little copy.

@andyfeller
Copy link

andyfeller commented Nov 4, 2024

I think this is best raised to the upstream as we have no influence how the upstream module is defined.

My understanding is that without this change we block updating zalando/go-keyring via Go modules. If that is not the case, then this is maybe not needed. I agree that it's questionable in terms of security and reliability to not rely on github.com paths when the module code is anyway on github.com.

Are you saying that newer releases of alessio/shellescape could not be retrieved via the previous method of specifying the module (github.com/alessio/shellescape v1.5.1)?

As far as I know, ☝️ should have continued to work irregardless of what the go.mod said the module's name actually was.

My concerns

If you look at the source of https://al.essio.dev/, it appears this is the alessio/alessio.github.io GitHub Pages repo that is making API calls to present information to the consumer:

<html>
  <body>
    <script>
      (async () => {
        const response = await fetch('https://api.github.com/repos/alessio/alessio.github.io/contents/');
        const data = await response.json();
        let htmlString = '<ul>';
        for (let file of data) {
          htmlString += `<li><a href="${file.path}">${file.name}</a></li>`;
        }
        htmlString += '</ul>';
        document.getElementsByTagName('body')[0].innerHTML = htmlString;
      })()
    </script>
  <body>
</html>

Again, this doesn't objectively add anything and raises questions about exactly what is sourced.

@mikkeloscar
Copy link
Member

What I mean is you get this error:

diff --git a/go.mod b/go.mod
index 8731939..a58030e 100644
--- a/go.mod
+++ b/go.mod
@@ -3,7 +3,7 @@ module github.com/zalando/go-keyring
 go 1.18

 require (
-       al.essio.dev/pkg/shellescape v1.5.1
+       github.com/alessio/shellescape v1.5.1
        github.com/danieljoos/wincred v1.2.2
        github.com/godbus/dbus/v5 v5.1.0
 )
go mod tidy
go: github.com/zalando/go-keyring imports
	al.essio.dev/pkg/shellescape: github.com/alessio/shellescape@v1.5.1: parsing go.mod:
	module declares its path as: al.essio.dev/pkg/shellescape
	        but was required as: github.com/alessio/shellescape
go: github.com/zalando/go-keyring imports
	github.com/danieljoos/wincred tested by
	github.com/danieljoos/wincred.test imports
	github.com/stretchr/testify/assert: github.com/alessio/shellescape@v1.5.1: parsing go.mod:
	module declares its path as: al.essio.dev/pkg/shellescape
	        but was required as: github.com/alessio/shellescape
go: github.com/zalando/go-keyring imports
	github.com/danieljoos/wincred tested by
	github.com/danieljoos/wincred.test imports
	github.com/stretchr/testify/mock: github.com/alessio/shellescape@v1.5.1: parsing go.mod:
	module declares its path as: al.essio.dev/pkg/shellescape
	        but was required as: github.com/alessio/shellescape

Again, this doesn't objectively add anything and raises questions about exactly what is sourced.

I don't disagree, but unless there is a practical way to work around it we can't without discussing these points with upstream (or inline the functions as @szuecs suggested).

@williammartin
Copy link
Contributor

I created #117 for inlining this. I think it's a good idea, but I also don't think it's the end of the world if we don't. I just thought that since we cared a bit in gh, that we should do the legwork to propose it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants