internal types package

[skattoju]

Description of PR

This PR moves the generic stack implemenetation currently in the capabilities package to a more generic place the current proposal being a types
package under the internal package.

Fixes #278

Changes (required)

• move generic stack implementation to internal/types package

Deprecations (optional)

Checklist (required)

• [*] I have reviewed and followed the contribution guidelines
• [*] I have performed a self-review of my code
• [*] I have commented my code, particularly in hard-to-understand areas
• [*] I have made corresponding changes to the documentation if needed
• [*] I have checked that my changes pass all tests

References (optional)

[exe-prow-github-app]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign yashoza19 for approval by writing /assign @yashoza19 in a comment. For more information see:The Kubernetes Code Review Process.

Associated issue: #101

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[exe-prow-github-app]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[skattoju]

/test all

[exe-prow-github-app]

@skattoju: No jobs can be run with /test all.
The following commands are available to trigger required jobs:

• /test echo-test

In response to this:

/test all

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.

[bcrochet]

Two things.

1. Are you thinking that this will be uses somewhere other than the capability package? That's not clear here.
2. I'm not a fan of the 'types' package. If anything, I'd rather see this turn into a 'stack' package.

[skattoju]

1. Are you thinking that this will be uses somewhere other than the capability package? That's not clear here.

Not seeing it at the moment but potentially yes.

2. I'm not a fan of the 'types' package. If anything, I'd rather see this turn into a 'stack' package.

The types package would have both options and stack types for now.. could call it util or something more generic. options is needed by both reports and capability (#294 )

Stack on the other hand although used only in capability is generic by design and even though it's not used anywhere today could be used in the future ? But not seeing it being used anywhere else at the moment as you pointed out..

[bcrochet]

1. Are you thinking that this will be uses somewhere other than the capability package? That's not clear here.

Not seeing it at the moment but potentially yes.

Until we do, I think it's fine to stay where it is.

2. I'm not a fan of the 'types' package. If anything, I'd rather see this turn into a 'stack' package.

The types package would have both options and stack types for now.. could call it util or something more generic. options is needed by both reports and capability (#294 )

Util is worse. :) And as I pointed out in #294, options should stay where it is.

Stack on the other hand although used only in capability is generic by design and even though it's not used anywhere today could be used in the future ? But not seeing it being used anywhere else at the moment as you pointed out..

Let's not prematurely reorganize things if there is no immediate need to do so.

[acmenezes]

Let's not prematurely reorganize things if there is no immediate need to do so.

@bcrochet @skattoju I would vote for a list package. That thing mimics a little bit a Python list somehow and could be improved to have other features. And I think that can actually be put on an actual pkg folder and not internal. It is useful anywhere already. This is the reason I opened the issue. Besides that, moving it won't harm the project in any way.

[bcrochet]

Let's not prematurely reorganize things if there is no immediate need to do so.

@bcrochet @skattoju I would vote for a list package. That thing mimics a little bit a Python list somehow and could be improved to have other features. And I think that can actually be put on an actual pkg folder and not internal. It is useful anywhere already. This is the reason I opened the issue. Besides that, moving it won't harm the project in any way.

1. I'm not sure that 'list' would be the right place either. list.Stack just seems... wrong.
2. Not a fan of 'pkg'. It's an artificial organization that a good swath of the Go community do not agree with.
3. Do we really want to start exposing things out of internal? That comes with having to maintain the API for external use. I'm not sure I want to go there.

[skattoju]

This would still be internal and wouldn't really change things up too much. Open to alternative suggestions to types.Stack could also close this one and leave things as is lmk what y'all think.

[skattoju]

Closing since there is no explicit need at this time.