🐛 Skip bundle defaults if --tag passed

[dev-drprasad]

What does this change

if --tag passed, porter still tries to validate local manifest file. This PR skips validating manifest file if --tag present

What issue does it fix

Closes #1180

Notes for the reviewer

N/A

Checklist

• Unit Tests
• Documentation - N/A
• Schema (porter.yaml) - N/A

If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

[dev-drprasad]

In draft, cuz thinking of adding unit tests

[vdice]

It looks like there are some test failures to investigate: https://dev.azure.com/deislabs/porter/_build/results?buildId=6704&view=results

I like how elegant the solution looks here, but it may need some adjustments? I sketched out another approach. Not sure if any of the ideas here are worth incorporating: https://github.com/deislabs/porter/compare/main...vdice:fix/defer-to-tag?expand=1

[dev-drprasad]

@vdice I feel, your solution directly convey the problem we're trying to solve. Let incorporate those changes only

[vdice]

Surprisingly, I couldn't get this test to fail when running against the current code on main (meaning, without the changes to cnab.go and lifecycle.go above). Maybe double-check my work.

I succeeded in getting it to fail when explicitly specifying opts.File = "porter.yaml" (and it didn't seem like the working dir logic was strictly necessary):

+++ b/pkg/porter/lifecycle_test.go
@@ -1,8 +1,6 @@
 package porter

 import (
-       "os"
-       "path"
        "path/filepath"
        "testing"

@@ -164,13 +162,11 @@ func TestBundleLifecycleOpts_ToActionArgs(t *testing.T) {
        t.Run("ignore manifest in cwd if tag present", func(t *testing.T) {
                opts := BundleLifecycleOpts{}
                opts.Tag = "deislabs/kubekahn:latest"
+               opts.File = "porter.yaml"

-               // cnab.go#defaultBundleFiles uses `os.Getwd()`
-               wd, _ := os.Getwd()
-               p.TestConfig.TestContext.AddTestFileContents([]byte(""), path.Join(wd, "porter.yaml"))
+               p.TestConfig.TestContext.AddTestFileContents([]byte(""), "porter.yaml")
                err := opts.Validate(nil, p.Porter)
                require.NoError(t, err, "Validate failed")
-               p.TestConfig.TestContext.FileSystem.Remove(path.Join(wd, "porter.yaml"))
        })
 }

We don't necessarily need to go with the test mods above, but we should make sure that whatever test we have fails without the actual code changes elsewhere in this PR.

[dev-drprasad]

Surprisingly, I couldn't get this test to fail when running against the current code on main

My bad! Thanks for catching this. Test case indeed failing in my local with main branch. But the reason was something else and I thought test case working as expected 🤦🏻‍♂️

opts.File = ''porter.yaml" simulates explicitly passing of --file arg. I wanted to test default behaviour without passing --file arg

[dev-drprasad]

I couldn't make this test fail with main branch with just one empty manifest file.

defaultBundleFiles#manifestExists will be true only if I use path.Join(wd.... and we need it to execute bundle default logic.
But manifest.go#readFromFile func gets only filename (not complete path) and reads manifest file from root directory. So, I had to use root manifest file also

[vdice]

Ah I see. The in-line comments help! Thanks for updating.

[vdice]

LGTM! Thank you @dev-drprasad