Move api/filesys to kyaml/filesys #3997
Conversation
|
@campoy: This PR has multiple commits, and the default merge method is: merge. 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. |
|
Hi @campoy. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
I just moved code from one package to another so I am afraid the tests breaking were breaking previously too for Windows. |
|
/ok-to-test |
As a workaround, can we add |
|
How about I just add something to skip windows? |
Sounds good to me! Thanks |
I agree this should not cause any new failures, and this is really strange. But master wasn't failing before, so how is this possible? Any ideas @natasha41575 ? Is there some other exclusion mechanism being used to skip the tests in their previous location? |
The windows check doesn't run all the tests for api, but does for kyaml. If you look at the check, it has jobs "Test kyaml" and "Test cmd/config", but no "Test api" which is odd. I'm not sure who set up these tests or if this is intentional. |
|
I'm adding a test that will necessarily fail on api/filesys to see whether this was being tested at all. |
|
This actually confirms that the "Go / Test Windows" precheck doesn't check 'api/filesys' |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: campoy, natasha41575 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
||
| // +build !windows | ||
|
|
||
| package filesys |
There was a problem hiding this comment.
Why the change to not have a separate test package here and in a few other files?
There was a problem hiding this comment.
I had the same question but (maybe incorrectly) assumed that it didn't matter. Was there a motivation for having them separate?
There was a problem hiding this comment.
As far as I'm aware, the main reason for separate test packages is to force the tests to be written against the public API, i.e. test from the perspective of the consumers of the package. I can't speak to whether that was the reason for the separate package in this case. Since this PR is just moving code from one place to another, I think it's fair to ask for an explanation of for any actual changes that were included. :)
Updates #3873
This PR moves all of the code in api/filesys to kyaml/filesys and updates all known dependencies to use the new package.
Instead of simply removing api/filesys, I kept a deprecated package (not the Deprecated: comment) so users can migrate at their pace.