Skip to content

Symlink internal dir in e2e #871

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

Merged
merged 2 commits into from
Dec 21, 2018

Conversation

estroz
Copy link
Member

@estroz estroz commented Dec 20, 2018

Description of the change: symlink both pkg and internal dirs to the test project's vendor when running e2e tests locally, and some cleanup.

Motivation for the change: the e2e tests symlink internal so any future changes are picked up by local e2e tests.

@estroz estroz added the kind/bug Categorizes issue or PR as related to a bug. label Dec 20, 2018
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 20, 2018
@estroz estroz changed the title Symlink remove cycles Symlink internal and remove import cycles Dec 20, 2018
@estroz estroz changed the title Symlink internal and remove import cycles Symlink internal in e2e and remove import cycles Dec 20, 2018
Copy link
Contributor

@AlexNPavel AlexNPavel left a comment

Choose a reason for hiding this comment

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

Is there a way to handle the scaffold constants better here? I'm worried that if we change a filename in our scaffolds, we might forget to do it in internal, which could break something. Hopefully E2E would catch that, but it's still possible that something would slip through a crack.

@estroz
Copy link
Member Author

estroz commented Dec 20, 2018

@AlexNPavel while scaffold constants should be used wherever possible to prevent code drift, they change infrequently enough that having internal use hard-coded strings shouldn't be a problem. Alternatively we can either pass constants in as arguments to functions that use them, although that's more cumbersome imo, or duplicate some code and have an internal dir for both commands and pkg. WDYT?

@AlexNPavel
Copy link
Contributor

I guess it's fine to leave the constants as they are. As you said, they shouldn't change often. And other solutions are a bit cumbersome.

@estroz estroz force-pushed the symlink-remove-cycles branch from 47164f7 to 58edae5 Compare December 20, 2018 23:21
@joelanford
Copy link
Member

Just a thought, but #756 is potentially related to this issue. If we moved pkg/scaffold into internal, would that solve the problem?

@estroz
Copy link
Member Author

estroz commented Dec 21, 2018

@joelanford yes. We can have projutil functions required by pkg/scaffold in the scaffold package so we don't need to import constants. Also none of the stuff in yamlutil/manifests.go is used by scaffolds, but I plan to use yamlutil/scan.go in another PR so that'll take some restructuring.

I will scrap the constants part of this PR in favor of restructuring and stick with just symlinking internal.

@estroz estroz force-pushed the symlink-remove-cycles branch from 58edae5 to 2ce77cf Compare December 21, 2018 00:23
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 21, 2018
@estroz estroz changed the title Symlink internal in e2e and remove import cycles Symlink internal dir in e2e Dec 21, 2018
Copy link
Contributor

@AlexNPavel AlexNPavel left a comment

Choose a reason for hiding this comment

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

LGTM

@estroz estroz merged commit 504b0cc into operator-framework:master Dec 21, 2018
@estroz estroz deleted the symlink-remove-cycles branch December 21, 2018 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants