cucumber / common

A home for issues that are common to multiple cucumber repositories
https://cucumber.io/docs
MIT License
3.36k stars 694 forks source link

Which data to show in Meta message ? #1003

Closed vincent-psarga closed 4 years ago

vincent-psarga commented 4 years ago

This is linked to what is on progress in #1002

Currently, the library provides the same information than the latest releases of fake-cucumber, cucumber-jvm and cucumber-ruby, being:

Ideally, this message will end up in tools used to provide Living documentations (like CucumberStudio, Cucumber4Jira and others), sos some extra informations would be nice. What I'm thinking of right now is:

Those changes will imply a new major release of cucumber-messages (and gherkin and other ones relying on messages), so it would be interesting to list as much as possible here and avoid doing multiple major releases of the library :)

tooky commented 4 years ago

I think the system ENV information would be really valuable meta information and would cover a bunch of that stuff.

There might be things in there that people don't want to send to a remote system though. So we could potentially remove or blank out any thing with _PASSWORD, _TOKEN, _SECRET.

mpkorstanje commented 4 years ago

@tooky I'd suggest using an explicit whitelist. So we only include well known values that we know to be safe.

I tried to do something similar with a blacklist a while ago and it was a never ending amount of pain. The blacklist is never complete and every miss is immediately a problem.

mpkorstanje commented 4 years ago

@vincent-psarga for a first selection I believe we can use whatever the pro plug-in was doing. I thought we were going to copy that intact.

tooky commented 4 years ago

@mpkorstanje I think a whitelist is a nice idea - it would be big and might need to expanded with new build envs.

If I remember the pro-plugin had a blacklist but allowed for additional matchers to be specified in a config file so users can add more if needed.

@vincent-psarga It makes me wonder if we should have a way for users to check what would be sent?

mpkorstanje commented 4 years ago

Mmh. I like the whitelist + matchers idea but that also sounds complicated and like a good idea for the next version.

Getting the build and source coordinates first would already be useful on it's own.

tooky commented 4 years ago

Agreed - simple first.

vincent-psarga commented 4 years ago

Ok, so the Metamessage might look like this:

message Meta {
  /**
   * The [SEMVER](https://semver.org/) version number of the protocol
   */
  string protocol_version = 1;

  // SpecFlow, Cucumber-JVM, Cucumber.js, Cucumber-Ruby, Behat etc.
  Product implementation = 2;

  // Java, Ruby, Node.js etc
  Product runtime = 3;

  // Windows, Linux, MacOS etc
  Product os = 4;

  // 386, arm, amd64 etc
  Product cpu = 5;

  CVS cvs = 6;
  repeated ENV envs = 7;

  // A product has a name and a version
  message Product {
    // The product name
    string name = 1;
    // The product version
    string version = 2;
  }

  message CVS {
    // Git, Mercurial ...
    string name = 1;
    // Remote servers urls
    repeated string remotes = 2;
    // Branch used
    string branch = 3;
    // Current commit
    Commit commit = 4;
    // Status of the repo
    repeated Staging = 5;

    message Commit {
      // ID or hash
      string id = 1;
      string author = 2;
      string text = 3;
    }

    message Staging {
      // Path to the file
      string path = 1;
      FileStatus statuss

      enum FileStatus {
        MODIFIED = 0
        ADDED = 1
        DELETED = 2
        UNTRACKED = 3
      }
    }
  }

  // Environment variable name and value
  message ENV {
    string name = 1;
    string value = 2;
  }
}

I've added a suggestion from @cbliard to all list the status of the git repository.

mpkorstanje commented 4 years ago

Can we get at the status of the git repo without using libraries? What purpose would it serve for the report?

vincent-psarga commented 4 years ago

Can we get at the status of the git repo without using libraries? What purpose would it serve for the report?

Not sure to be honest if that's doable without an external lib (I haven't checked). That said, I'd like to see if there's simple libs to extract some informations to avoid re-inventing the wheel (especially if they've already handled weird cases).

Normally with a CI system pushing the messages this would be empty (although I would not be surprised seeing files modified for the CI to work :D ) but it could be interesting in local dev'.

mpkorstanje commented 4 years ago

That doesn't sound like it is very useful. If possible I'd like to avoid the complexity of dealing with the git status al together -- extracting the existing CI variables for all the different CI systems will be complex enough as is already. Can we leave the git status out until we have a good use case for it?

vincent-psarga commented 4 years ago

ok works for me, I'll start with the ENV variable first :) and we'll see how long that takes before considering git informations :)

mpkorstanje commented 4 years ago

Rather then using all environment variables, please look at old pro plugin. It gives us `

CIEnvironment(String ciName, String revision, String branch, String tag, String projectName)

I think most of that is pretty good. Though either or both tag and branch may need to be list of strings. A single commit can have multiple tags and branches.

vincent-psarga commented 4 years ago

indeed that seems like a better idea, I'll change that a bit.

mpkorstanje commented 4 years ago

I ran into a rather interesting one in slack. The code below may throw an NPE when shaded into a different jar.

Messages.Meta.newBuilder()
                        .setProtocolVersion(Messages.class.getPackage().getImplementationVersion())

When using shading maven will repackage all the class files into a new jar. This removes the information from the manifest. Or worse it may replace it with some other information.

This means that to get a hold of the implementation version we can not use Messages.class.getPackage().getImplementationVersion(). Rather we have to use a resource bundle that contains the version. e.g:

    private static final String VERSION = ResourceBundle.getBundle("io.cucumber.core.version")
            .getString("cucumber-jvm.version");

src/main/resource/io/cucumber/core/version.properties

cucumber-jvm.version=${parent.version}
gherkin.version=${gherkin.version}

Note: The ${parent.version} is interpolated by maven when building the jar.

The worst part is, I knew this.

Edit: Fixed this in JVM for now. Might be good to keep in mind if do decide to generate the entire message as part of this library.

https://github.com/cucumber/cucumber-jvm/commit/40657a683bfe922a02d745a3b8fba4a54c1b4d05

vincent-psarga commented 4 years ago

So that means that this would tighly couple this library with cucumber-jvm (or expect that this file with the property exists).

Would it be ok if I added a second entry point: createMeta(String toolName, String toolVersion, String messagesVersion) so in cucumber-jvm you specify the messages version like you currently fixed it ?

mpkorstanje commented 4 years ago

Not necessarily, we can also include a resources bundle in messages. But only for the version of messages. The version of Cucumber JVM and the fact that it is JVM will have to be provided externally.

vincent-psarga commented 4 years ago

The version (and name) of the message emitter (cucumber-jvm, fake-cucumber...) was already not included in the lin but provided by the emitter.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

stale[bot] commented 4 years ago

This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective.