[Suggestion] - Nested setCommits & readme fixes

[kamerat]

After the beautiful work on the pr #139 finally got released, I implemented it to my org's codebase but in the process, I felt it was missing something:

1. Nesting/Grouping of setCommits options

The new options: commit, previousCommit, repo, auto feels a bit polluting in the sentry-webpack-plugin's "global" options scope in my opinion. The key annoying me the most is auto.
auto has no context when seen like this:

const sentryCliPlugin = new SentryCliPlugin({
      release: '42',
      auto: true,
});

My suggestion is to group the setCommits/git options in a object:

const sentryCliPlugin = new SentryCliPlugin({
      release: '42',
      setCommits: {
          auto: true,
          //Other setCommits options here...
      }
});

I modified the code in index.js to reflect this and tried to do the same in the tests. However, the tests are not passing. I have no experience with writing tests sadly, so I do not see what I can do to make them pass. Logging this.options.setCommits gives expecting result 🤷‍♂. Hopefully someone can help me out here if they think this PR is reasonable.

2. Readme changes

I do not know if there is a typo here or if I'm too tired to see that i'm wrong, but it seems to me that the key: repo :

sentry-webpack-plugin/README.md

Line 115 in 5363d16

* `repo [optional]` - `string`, the full repo name as defined in Sentry

is indeed not optional:

sentry-webpack-plugin/src/index.js

Line 327 in 5363d16

if (repo && (commit || auto)) {

I don't know about the others, but I think we should do a deeper check to see what is required and what is not.

Yet again, awesome PR! I have been waiting for this for a long time ❤️

[kamerat]

Thoughts, @dmitryrn?

[kamilogorek]

I was thinking about doing that, however, I let it be as is, because this is how sentry-cli implements it – getsentry/sentry-cli@4b913f0#diff-1f9d9d0fe3b1cdb764fff6a8b443cb7dR49-R54

It'd be breaking change now, so we'd have to allow for both versions and just document grouped one. This'd require a change here:

const { commit, previousCommit, repo, auto } = this.options.setCommits || this.options;

[kamilogorek]

Now that your updated README table is merged, can you include the update in this commit as well?

[kamerat]

Now that README includes a table, can you include the update in this commit as well?

It would be my honor. 🛡

[dmitryrn]

I would suggest to add test for both versions of calling SentryCliPlugin({ ..., commit, previousCommit, repo }) and SentryCliPlugin({ ..., setCommits: { commit, previousCommit, repo } })

[kamerat]

That sounds like a good idea! 🎉
Sadly, I don't know how to write them 😭

[dmitryrn]

I guess this should be fine (from line 231):

  test('test setCommits with commits range', done => {
    const sentryCliPlugin = new SentryCliPlugin({
      include: 'src',
      release: '42',
      commit: '4d8656426ca13eab19581499da93408e30fdd9ef',
      previousCommit: 'b6b0e11e74fd55836d3299cef88531b2a34c2514',
      repo: 'group / repo',
    });

    sentryCliPlugin.apply(compiler);

    setImmediate(() => {
      expect(mockCli.releases.setCommits).toBeCalledWith('42', {
        repo: 'group / repo',
        commit: '4d8656426ca13eab19581499da93408e30fdd9ef',
        previousCommit: 'b6b0e11e74fd55836d3299cef88531b2a34c2514',
      });
      expect(compilationDoneCallback).toBeCalled();
      done();
    });
  });

  test('test setCommits with auto option', done => {
    const sentryCliPlugin = new SentryCliPlugin({
      include: 'src',
      release: '42',
      repo: 'group / repo',
      auto: true,
    });

    sentryCliPlugin.apply(compiler);

    setImmediate(() => {
      expect(mockCli.releases.setCommits).toBeCalledWith('42', {
        repo: 'group / repo',
        auto: true,
      });
      expect(compilationDoneCallback).toBeCalled();
      done();
    });
  });

  test('test setCommits with commits range under setCommits object', done => {
    const sentryCliPlugin = new SentryCliPlugin({
      include: 'src',
      release: '42',
      setCommits: {
        commit: '4d8656426ca13eab19581499da93408e30fdd9ef',
        previousCommit: 'b6b0e11e74fd55836d3299cef88531b2a34c2514',
        repo: 'group / repo',
      },
    });

    sentryCliPlugin.apply(compiler);

    setImmediate(() => {
      expect(mockCli.releases.setCommits).toBeCalledWith('42', {
        repo: 'group / repo',
        commit: '4d8656426ca13eab19581499da93408e30fdd9ef',
        previousCommit: 'b6b0e11e74fd55836d3299cef88531b2a34c2514',
      });
      expect(compilationDoneCallback).toBeCalled();
      done();
    });
  });

  test('test setCommits with auto option under setCommits object', done => {
    const sentryCliPlugin = new SentryCliPlugin({
      include: 'src',
      release: '42',
      setCommits: {
        repo: 'group / repo',
        auto: true,
      },
    });

    sentryCliPlugin.apply(compiler);

    setImmediate(() => {
      expect(mockCli.releases.setCommits).toBeCalledWith('42', {
        repo: 'group / repo',
        auto: true,
      });
      expect(compilationDoneCallback).toBeCalled();
      done();
    });
  });

[kamerat]

Thank you! I just used the "easy way" by using the patch posted below. Is there something you would like to add on that, or are we good? :D

[dmitryrn]

We are good 🚀

[kamilogorek]

Sadly, I don't know how to write them 😭

tests.patch.zip

@BlitZz96 Download, unzip, git apply <path-to-tests.patch> :)

[kamilogorek]

Also call yarn fix before pushing new changes, it'll fix linting issues for you and CI will be green again

[kamilogorek]

10/10 cooperation skills. v1.9.1 released. Thanks! 🙇‍♂️