Skip to content
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

Ignore meta resources in fn source #2139

Merged
merged 7 commits into from
Jun 7, 2021

Conversation

Shell32-Natsu
Copy link
Contributor

close #2033

  • Meta resources will be ignored by default
  • --include-meta-resources flag is used to explicitly include meta resources.


fnConfigPaths, err := FunctionConfigFilePaths(pkgPath, true)
if err != nil {
return nil, fmt.Errorf("failed to get pipeline config file paths: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

what's "pipeline config file"? Please audit the error message as usual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

thirdparty/cmdconfig/commands/cmdsource/cmdsource.go Outdated Show resolved Hide resolved

set -eo pipefail

# create a temporary directory
Copy link
Contributor

Choose a reason for hiding this comment

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

the test harness should create and delete the tmp dir automatically if bash scripts are provided. That eliminates lines 18,19,29,30. Can be a follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This temp dir is only used in out-of-place test cases because it needs a directory as another place to sink. I don't sink test harness should care about this kind of details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@droot has a PR for this #2163

Copy link
Contributor

Choose a reason for hiding this comment

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

We discovered a bit late that using .krmignore and special-dir (out for out-of-place), these tests could be written differently where exec.sh scripts deleting/copying files around to get the git diff to right thing. If we have that pattern working well for our testing, we can potentially switch to that style in a follow up PR. I am fine having these as-is for now.

# See the License for the specific language governing permissions and
# limitations under the License.

set -eo pipefail
Copy link
Contributor

Choose a reason for hiding this comment

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

harness should set this automatically. Can be a follow up.

Copy link
Contributor Author

@Shell32-Natsu Shell32-Natsu Jun 4, 2021

Choose a reason for hiding this comment

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

I suspect that this can only be set in the script. We may need a wrapper script to do it?


kpt fn source \
| kpt fn eval - --image gcr.io/kpt-fn/set-namespace:v0.1.3 -- namespace=staging \
| kpt fn sink .
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need the '.' , it's the default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DIR is required now as mentioned in #2032 (comment).

Create #2159 to track it.


fnConfigPaths, err := FunctionConfigFilePaths(pkgPath, true)
if err != nil {
return nil, fmt.Errorf("failed to get config paths for the functions in pipeline: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before. "config paths" won't mean much to the user. Are you referring to values specified in configPath? It's a specific field name with a proper noun.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to directly return error since wrapping it here looks not very useful.

@@ -29,6 +33,8 @@ func GetSourceRunner(name string) *SourceRunner {
}
c.Flags().StringVar(&r.FunctionConfig, "fn-config", "",
"path to function config file.")
c.Flags().BoolVar(&r.IncludeMetaResources,
"include-meta-resources", false, "include package meta resources in the command output")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does value of flag help message differ from reference docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The help message is a brief description about the flag, meanwhile the reference doc includes details. This is the same for other commands.

Copy link
Contributor

@frankfarzan frankfarzan Jun 4, 2021

Choose a reason for hiding this comment

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

I don't agree with this. It's weird to see to a different flag description from (--help) and reference docs. It's also a waste of time to describe the same thing twice. @mortent @droot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current output of --help combines the reference doc and flag description. Isn't it verbose to have same sentences twice?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually the real bug from an end user's perspective is that flags help gets printed twice in two different forms (detailed form picked from reference/*/.md and short form from the default commandline flags here).

I don't know the real fix here, but @Shell32-Natsu can you file a bug for this, we need to address it for all the commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frankfarzan
Copy link
Contributor

lgtm. I'll let Sunil approve.


set -eo pipefail

# create a temporary directory
Copy link
Contributor

Choose a reason for hiding this comment

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

We discovered a bit late that using .krmignore and special-dir (out for out-of-place), these tests could be written differently where exec.sh scripts deleting/copying files around to get the git diff to right thing. If we have that pattern working well for our testing, we can potentially switch to that style in a follow up PR. I am fine having these as-is for now.

@@ -29,6 +33,8 @@ func GetSourceRunner(name string) *SourceRunner {
}
c.Flags().StringVar(&r.FunctionConfig, "fn-config", "",
"path to function config file.")
c.Flags().BoolVar(&r.IncludeMetaResources,
"include-meta-resources", false, "include package meta resources in the command output")
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually the real bug from an end user's perspective is that flags help gets printed twice in two different forms (detailed form picked from reference/*/.md and short form from the default commandline flags here).

I don't know the real fix here, but @Shell32-Natsu can you file a bug for this, we need to address it for all the commands.

@Shell32-Natsu
Copy link
Contributor Author

@frankfarzan Looks like now all PRs need approval from code owner?

@frankfarzan
Copy link
Contributor

@phanimarupaka should include this in the migration doc.

@Shell32-Natsu Shell32-Natsu merged commit c7acd74 into kptdev:next Jun 7, 2021
@Shell32-Natsu Shell32-Natsu deleted the fn-source-meta branch June 7, 2021 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kpt fn source should handle meta resources similar to fn eval
3 participants