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

Implement check command #31

Closed
wants to merge 3 commits into from
Closed

Implement check command #31

wants to merge 3 commits into from

Conversation

mtojek
Copy link
Contributor

@mtojek mtojek commented Jul 30, 2020

Fixes: #10

Note: some commands are placeholders, they will be implemented in another issues.

@mtojek mtojek added the Team:Integrations Label for the Integrations team label Jul 30, 2020
@mtojek mtojek requested a review from ycombinator July 30, 2020 08:35
@mtojek mtojek self-assigned this Jul 30, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@exekias
Copy link

exekias commented Jul 30, 2020

What's the point of adding a placeholder? Why not waiting for the implementation to have it exposed in the cmdline?

@mtojek
Copy link
Contributor Author

mtojek commented Jul 30, 2020

The added value in this PR is the composition of multiple targets and the expected order. I suggest to look at this as a skeleton defining the development process.

It would be easier to test if we have a single command "check" for the integration, which will go through multiple goals. For example: if you implement a command "validate", all you have to do is to run "check" which will iterate over other targets (build, format, etc.) and verify if there are still correct.

This repository is private currently, so I hope it won't introduce much noise.

@mtojek
Copy link
Contributor Author

mtojek commented Jul 30, 2020

Another pros -

Imagine we decide to integrate it with integrations: elastic-package check and no need to touch integrations later on and simply deliver next features of the elastic-package.

cmd/check.go Show resolved Hide resolved
@exekias
Copy link

exekias commented Jul 30, 2020

Thanks I understand this better now. I have a few concerns about this:

  • including test as part of check. Wouldn't this make check too slow? This may be ok if that's what we are looking for
  • In general I have the feeling that we are creating too many validation subcommands (validate, check, format). Would be nice to converge them to make this simpler

@mtojek
Copy link
Contributor Author

mtojek commented Jul 30, 2020

We had an offline discussion, agreed on the following approach:

  • check - aggregates multiple commands (format, lint, build)
  • test - keep it aside
  • validate becomes lint

@mtojek
Copy link
Contributor Author

mtojek commented Jul 30, 2020

I think Github is facing an outage right now. Missing commits (already pushed) here: https://github.com/mtojek/elastic-package/commits/10-check-integration

func setupCheckCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "check",
Short: "Check the integration",
Copy link

Choose a reason for hiding this comment

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

Sorry for hijacking this PR but I think we could make a more consistent use of the terms package and integration.

I understand that at this point in time integrations represent the only available type of package but this will hopefully change in the future.

So we should decide whether this tool is somehow dedicated to the development of specific packages of type: integration, and in this case I would change the name of the tool and maybe move the source code into the elastic/integrations repo; or we say this is a general purpose tool to support the development of packages, and in this case I'd consolidate the naming scheme around source code, docs, etc. avoiding to refer to integrations when the command can actually work with any package type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @masci, thanks for looking into this! I believe we have a discussion about this in the past.

We decided to move on with packages, but as long as there are no other types even defined (or in plans), I sticked to integrations. I can try to get rid of words referring to "integrations" in as many places as possible, but internally there will be support only for integrations.

I prefer to keep this in a separate repository as the original plan is to keep integration source in the integrations repo and nothing else (no logic, tooling, side scripting).

I'd like to listen to other opinions on this. @exekias @ruflin @ycombinator

Copy link

Choose a reason for hiding this comment

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

I'm +1 on having this tool as the reference for package development, where integrations will be only a subset of that. In that sense I would refrain from mentioning "integrations" here.

In that sense I think it also makes sense to have the tool in a different repo, not only because of this "generic" soul, but also because I expect that once things get stable, elastic-package users won't necessarily be go developers (with the go toolchain installed for instance). This project should have its own lifecycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't mind I would go over this in another PR. It's not directly in the scope of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#35

@mtojek
Copy link
Contributor Author

mtojek commented Jul 30, 2020

Resolving in favor of #34

@mtojek mtojek closed this Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command: check the integration
4 participants