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

Cleanup and docs #60

Merged
merged 6 commits into from
Oct 31, 2022
Merged

Cleanup and docs #60

merged 6 commits into from
Oct 31, 2022

Conversation

cmurphy
Copy link
Contributor

@cmurphy cmurphy commented Oct 17, 2022

  • Remove unused packages and functions
  • Cleanup minor code quality issues
  • Fix HTTP compliance
  • Add godoc comments for the stores/partition and stores/proxy packages. This is to set up a base for significant alterations to how the base stores work with pagination, sorting, and filtering.

Partially addresses rancher/rancher#38593

Copy link
Contributor

@rmweir rmweir left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

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

Thanks for adding comments to a lot of this stuff.

I have some concerns about changing exported functions/interfaces on a non-major release. I'd prefer we not do this (or that we tag the code after this is merged as v1.0.0) - as we are not the only users of steve.

One other note - your CI is going to fail until #59 goes in. If you want it to pass, you can update a specific part of the .drone.yml to fix that - see here for what you need to add.

pkg/proxy/proxy.go Show resolved Hide resolved
pkg/stores/proxy/rbac_store.go Show resolved Hide resolved
pkg/stores/switchstore/store.go Show resolved Hide resolved
pkg/server/router/router.go Show resolved Hide resolved
@cmurphy
Copy link
Contributor Author

cmurphy commented Oct 19, 2022

@MbolotSuse I disagree about changing exported functions. steve does not follow semver. It does not even have releases. We don't make any guarantees of backwards compatibility. I've checked in harvester and throughout github and nothing is using these functions and packages. If I missed something, someone can file an issue and it is easy to revert. But I don't think trying to live up to an idealistic standard of API compatibility is helpful to us. It is more beneficial to clean up cruft because this helps us maintain the code in the long run.

@MbolotSuse
Copy link
Contributor

Discussed offline - since Steve doesn't currently follow a versioning strategy, there's no incentive for us to preserve functionality between changes. Especially when the functionality isn't being used by other areas.

@MbolotSuse MbolotSuse self-requested a review October 20, 2022 16:20
Copy link
Contributor

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@KevinJoiner KevinJoiner left a comment

Choose a reason for hiding this comment

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

"[s]he is a hero, not the hero we deserved but the hero we needed"
🙇‍♂️

Remove duplicate import and make aliasing of other apiserver imports
consistent throughout steve.
HandlerFromConfig was not used in steve or rancher.
The header defined in RFC 9110[1] is "Accept", not "Accepts". This
change corrects the route filter. Since this API is not documented and
the header was plainly incorrect, no attempt is made at backwards
compatibility.

[1] https://www.rfc-editor.org/rfc/rfc9110.html#name-accept
Usage of the switchstore store was removed when the clusters resource
was removed in dcea1db.
The use of AddNamespaceConstraint was removed in e35b8304 of
rancher/rancher, so there is no possibility of there being a namespace
constraint in the request context. Remove the unused function and the
unused codepath from the rbac store.
@cmurphy
Copy link
Contributor Author

cmurphy commented Oct 27, 2022

rebased

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.

4 participants