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

Remove sget command #1364

Closed
wants to merge 1 commit into from
Closed

Remove sget command #1364

wants to merge 1 commit into from

Conversation

luhring
Copy link
Contributor

@luhring luhring commented Jan 26, 2022

Summary

Removes the sget command for now, while the team refines the vision for the "sget" idea, how it will be manifested in code, and how we'll distribute it to users.

Ticket Link

Related to #1363

Release Note

Removes sget command

Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

@luhring
Copy link
Contributor Author

luhring commented Jan 26, 2022

Thanks @cpanato for finding those! 🙏

I also found a handful of other spots we're using the sget command, and I removed them. Let me know if this was too heavy-handed.

Also, I left the sget Go package for now, since it's used in cosign policy init and in E2E testing. Let me know if you think we should work to remove this "sget" reference as well.

cpanato
cpanato previously approved these changes Jan 26, 2022
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

LGTM

will defer to @dlorenc

@cpanato
Copy link
Member

cpanato commented Jan 26, 2022

would merge this after the 1.5.1 release

@dlorenc
Copy link
Member

dlorenc commented Jan 26, 2022

I'd honestly still rather wait here until we have a plan. I don't think it's going to take forever to get something drafted, but doing it in a rush before we've figured out what the next step is feels like a mistake.

@luhring
Copy link
Contributor Author

luhring commented Jan 26, 2022

I'd honestly still rather wait here until we have a plan. I don't think it's going to take forever to get something drafted, but doing it in a rush before we've figured out what the next step is feels like a mistake.

Agreed 💯 . Let's do that, and this PR is here if/when we want it.

@dlorenc
Copy link
Member

dlorenc commented Feb 2, 2022

Ok so for a plan, we can leave this around until the new repo is created and we've done a release from there. That way we can delete here without causing too much confusion.

@jeff-mccoy
Copy link

Thanks @dlorenc...we already included the sget package in our go project not realizing it was possibly being removed 😳. Thanks to @imjasonh for pointing this out to us in slack.

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@imjasonh
Copy link
Member

imjasonh commented Sep 6, 2022

@dlorenc ping, possibly #2019 first?

@github-actions
Copy link

github-actions bot commented Oct 8, 2022

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@imjasonh
Copy link
Member

I think we can do this now.

#2019 was merged in 1f42a24 and released as part of cosign 1.12. We're on 1.13 now, so I think this is ample warning for folks to stop depending on it.

WDYT @dlorenc

@cpanato
Copy link
Member

cpanato commented Oct 13, 2022

@imjasonh, when we make the release of the sget in the other repo we will continue the release version of sget and do a v1.14.0 lets say or do you want to bump to another version?

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@cpanato
Copy link
Member

cpanato commented Nov 13, 2022

Not stale

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@cpanato
Copy link
Member

cpanato commented Feb 14, 2023

@luhring @imjasonh @dlorenc any plans on this? should we move forward or close this PR?

@znewman01
Copy link
Contributor

I think we should merge this PR. As-is, sget is only marginally useful. There's no real replacement but then again sget itself was never complete and often encouraged security antipatterns.

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@luhring
Copy link
Contributor Author

luhring commented Mar 28, 2023

I think we should merge this PR.

Just catching up here! 😅 I'm happy to go through the merge conflicts and resolve them if current maintainers are still into this change! Otherwise we can close this.

@cpanato
Copy link
Member

cpanato commented Mar 28, 2023

i think we want that, the sget repo was archived and I think we should remove this as well

I can help here as well @luhring in case you need

cc @priyawadhwa @bobcallaway @lukehinds thoughts?

@priyawadhwa
Copy link
Contributor

Removing it sgtm!

@luhring
Copy link
Contributor Author

luhring commented Mar 29, 2023

I can help here as well @luhring in case you need

@cpanato That'd be great, actually, thank you! I just tried to resolve conflicts (and I'll push this state of my branch here), but it looks like there are additional uses of sget that have popped up since this PR was first opened (seeing several more references with grep -Rni 'sget' .).

I defer to you on how best to move forward.

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #1364 (912848f) into main (5b8af9c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1364   +/-   ##
=======================================
  Coverage   29.47%   29.47%           
=======================================
  Files         151      151           
  Lines        9678     9678           
=======================================
  Hits         2853     2853           
  Misses       6386     6386           
  Partials      439      439           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

- id: linux-cosigned
binary: cosigned-linux-{{ .Arch }}
no_unique_dist_dir: true
main: ./cmd/cosign/webhook
Copy link
Member

Choose a reason for hiding this comment

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

from where this come from? this was deprecated a long time and not have it own repo (policy-controller)

@cpanato
Copy link
Member

cpanato commented Apr 6, 2023

@luhring maybe you rebased using an older base?
it is okay to close this and I open a fresh one?

@luhring
Copy link
Contributor Author

luhring commented Apr 6, 2023

@cpanato Please! 🙏

@cpanato cpanato mentioned this pull request Apr 9, 2023
@cpanato
Copy link
Member

cpanato commented Apr 9, 2023

closing in favor of #2885

@cpanato cpanato closed this Apr 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants