[israel-hdez]

Description

• Go back to using IMG ?= rather than forcing the value with IMG =. This makes it easier to override the value of IMG for development, and also aligns it to how BUNDLE_IMG is assigned.
• Add prepare as dependency of the undeploy target. This is just for convenience, although when looking at the other targets, prepare may simply be a missing dependency in undeploy.

How Has This Been Tested?

Using the Makefile :-)

Merge criteria:

• The commits are squashed in a cohesive manner and have meaningful messages.
• Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has manually tested the changes and verified that the changes work

[ykaliuta]

• Go back to using IMG ?= rather than forcing the value with IMG =.

Doubtful. It's not forcing, it's assigning. Forcing is with override keyword.

This makes it easier to override the value of IMG for development,

What difficulties do you have?

The only scenario I can imagine is setting environment variable and running make without -e switch. And IMG is pretty bad name for environment variable. All other scenarios (command line, local.mk and even make -e which wrong usage is all around different documentations) work fine.

and also aligns it to how BUNDLE_IMG is assigned.

Align it :). I frankly cannot imagine any usecase where most of our ?= would be useful (well, some should be moved before the -include local.mk to say any instead of most).

I should remind you, that command line overriding has preference over most of assignments https://www.gnu.org/software/make/manual/html_node/Values.html

• Add prepare as dependency of the undeploy target. This is just for convenience, although when looking at the other targets, prepare may simply be a missing dependency in undeploy.

Hmm, debatable. I would prefer for undeploy to have the same configuration I had for deploy. But would listen to others. From the description it seems that you did not have any practical problem, right?

[israel-hdez]

What difficulties do you have? The only scenario I can imagine is setting environment variable and running make without -e switch.

The scenario you mention is what I want.

Because of the number of repos I use, I like using direnv to remember less. I do can use the -e switch, but it would be something to remember (on the other repos I use, I don't need any flags). I think I still can use IMAGE_TAG_BASE and IMG_TAG as a proxies, but I thought it was a regression because in the past it was possible to simply set the IMG envvar, similarly to BUNDLE_IMG and CATALOG_IMG.

And IMG is pretty bad name for environment variable.

I would agree.

Hmm, debatable. I would prefer for undeploy to have the same configuration I had for deploy. But would listen to others.

I think this makes sense, although there is an implied workflow: to deploy (which would also prepare), then undeploy. Nothing wrong.

Use-case: if you share a cluster with another dev, they won't have the generated artifacts and undeploy won't work correctly, until you first do prepare. So, I think I'd appreciate a note in the README, as there are no pointers that undeploy is broken if you hadn't ran deploy or prepare previously.

From the description it seems that you did not have any practical problem, right?

Not a problem, as I could simply do make prepare undeploy.

It is just that since manifests get modified as part of development (nothing wrong with this), that I often do hard-resets for clean-up. Then trying to undeploy in an existent cluster would not work until I do prepare. So, as I said in my main comment, this is more for convenience.

I guess this PR is more on the enhancements side, than on the fixes side. I anyway have workarounds, so feel free to drop the PR if you think there is nothing to enhance :-)

[israel-hdez]

Hmm, debatable. I would prefer for undeploy to have the same configuration I had for deploy.

BTW, I find weird that for undeploy you would need to run prepare manually, while uninstall has prepare as dependency.

[ykaliuta]

What difficulties do you have? The only scenario I can imagine is setting environment variable and running make without -e switch.

The scenario you mention is what I want.

Because of the number of repos I use, I like using direnv to remember less. I do can use the -e switch, but it would be something to remember (on the other repos I use, I don't need any flags). I think I still can use IMAGE_TAG_BASE and IMG_TAG as a proxies, but I thought it was a regression because in the past it was possible to simply set the IMG envvar, similarly to BUNDLE_IMG and CATALOG_IMG.

Ah, great! So, I will rephrase the commit message for myself to something like:

Makefile: revert IMG assigned type to prefer environment

This revert the change introduced by
a799964cf6df ("Makefile: generate default image name based on current user (#730)")
since the discarded scenario explicitly described there actually is in use, so it introduced a regression.  

Regression definitely was not an intention.

And IMG is pretty bad name for environment variable.

I would agree.

Hmm, debatable. I would prefer for undeploy to have the same configuration I had for deploy. But would listen to others.

I think this makes sense, although there is an implied workflow: to deploy (which would also prepare), then undeploy. Nothing wrong.

Use-case: if you share a cluster with another dev, they won't have the generated artifacts and undeploy won't work correctly, until you first do prepare.

Sounds as other side of the coin. But the other party better know the variables you called prepare with to generate same configuration, doesn't? The same was valid for my local regeneration.

No, I do not have a strong opinion what is better. Both more or less work for me.

So, I think I'd appreciate a note in the README, as there are no pointers that undeploy is broken if you hadn't ran deploy or prepare previously.

From the description it seems that you did not have any practical problem, right?

Not a problem, as I could simply do make prepare undeploy.

It is just that since manifests get modified as part of development (nothing wrong with this), that I often do hard-resets for clean-up. Then trying to undeploy in an existent cluster would not work until I do prepare. So, as I said in my main comment, this is more for convenience.

I guess this PR is more on the enhancements side, than on the fixes side. I anyway have workarounds, so feel free to drop the PR if you think there is nothing to enhance :-)

It has 2 pretty different changes actually.

[ykaliuta]

Hmm, debatable. I would prefer for undeploy to have the same configuration I had for deploy.

BTW, I find weird that for undeploy you would need to run prepare manually, while uninstall has prepare as dependency.

Agree. But it was done originally for some reason (prepare just replaced the existing generation parts).

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ykaliuta

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ykaliuta]

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

[israel-hdez]

Sounds as other side of the coin.

Definitely! Although, it was just an example use-case. Emphasis on just an example, no need to cover it :-)

But the other party better know the variables you called prepare with to generate same configuration, doesn't?

Yup... Although, right now, I think it doesn't matter for undeploy isn't it?. But I do understand the need of syncing on the values as it could be relevant for other uses.

---

And thanks for moving this forward :-)