Skip to content

cleanup: call install in the sample target #4057

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

Closed
wants to merge 1 commit into from
Closed

cleanup: call install in the sample target #4057

wants to merge 1 commit into from

Conversation

camilamacedo86
Copy link
Contributor

Description of the change:

  • instead of calling the make install in the sanity add it to the samples target

Motivation for the change:

  • make it easier for contributors since they can face issues just because forgot to run make install before to re-gen the samples.

Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2020
@joelanford
Copy link
Member

I believe this is already resolved in #4023. I’d prefer we capture all Makefile related changes there to avoid competing PRs and rebases.

@camilamacedo86
Copy link
Contributor Author

Hi @joelanford,

This PR is very specific to address the action to instead of calling the make install in the sanity add it to the samples target in order to make it easier for contributors since they can face issues just because forgot to run make install before to re-gen the samples. So, I do not see any problem to not get it merged.

However, I am fine to do that in #4023 . Btw it is NOT done in the #4023

@joelanford
Copy link
Member

Btw it is NOT done in the #4023

Can you explain what's missing?

@camilamacedo86
Copy link
Contributor Author

closing it in favor of #4023

@camilamacedo86 camilamacedo86 deleted the makesamplesinstall branch October 19, 2020 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants