[CCR] Post auto follow pattern endpoint

[sebelga]

Add API route to create a Cross Cluster Replication auto-follow pattern. In this PR (de)serialization has been added to both the API list() and create() routes so we can work with camelCase naming on the client.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[cjcenizal]

This looks great! I reviewed the code and tested locally. I had a lot of really minor comments. I like how you have the de/serialization logic on the server. It creates a nice abstraction layer over ES. In retrospect I should have done that with the rollup jobs API as well.

I found a couple UI tweaks we could make. In the empty state, I think we can remove the title, to bring it inline with the empty states of other apps. To tie the language back to the name of the app, I think we could change the title of the empty prompt itself to "Cross cluster replication" and the description to "Auto-follow patterns replicate leader indices from a remote cluster to follower indices on the local cluster."

Then for the regular table state, we can remove the subtitle "Auto-follow patterns", and replace "Manage your auto follow patterns" with the description from the empty prompt. I think this will make the two states feel more cohesive and simplify the concepts we're communicating. What do you think?

Also, can we use "Create" instead of "Add" for the action? I think this is something the copy writers have been trying to make consistent.

[cjcenizal]

I notice that some files are named autofollow and some are named auto_follow. I think we should standardize one. I would lean towards auto_follow since this would reflect the casing of the variable names, e.g. autoFollow.

[sebelga]

Totaly agree no that... I have also some translation key that I need to camelCase for consistency. 👍

[cjcenizal]

Could we add the specific rule being disabled here?

[sebelga]

👍

[cjcenizal]

I would find it a bit easier to read this if we indented the closing parens, which I think is a pattern you've used elsewhere:

export const getAutoFollowPatternMock = (
  remoteCluster = chance.string(),
  leaderIndexPatterns = [chance.string()],
  followIndexPattern = chance.string()
) => ({
  remote_cluster: remoteCluster,
  leader_index_patterns: leaderIndexPatterns,
  follow_index_pattern: followIndexPattern
});

[sebelga]

Good catch 👍

[cjcenizal]

Small typo: "deSerialize" -> "deserialize"

[sebelga]

👍

[cjcenizal]

A few small typos in this file: "deSerialize" -> "deserialize"

[sebelga]

👍

[cjcenizal]

Should these method names be Autofollow or AutoFollow? I lean towards the latter to be consistent with the ES docs, but either way I think we should be consistent.

[sebelga]

Yes, should be AutoFollow 👍

[cjcenizal]

Also it looks like this method name is missing a "w": "Autofollo" -> "Autofollow"

[sebelga]

👍

[cjcenizal]

Minor nit, but it seems like unnecessary work to create a new object and repopulate it every iteration. Is there a disadvantage to reusing the accumulator like this instead?

  Object.entries(autoFollowPatterns).reduce(
    (deserializedPatterns, [name, autoFollowPattern]) => {
      deserializedPatterns[name] = deserializeAutoFollowPattern(name, autoFollowPattern);
      return deserializedPatterns;
    },
    {}
  );

[sebelga]

It is just for code style, to avoid the returnstatement. True that in a long list with multiple request/seq this would not be efficient but in our case it would not make any difference

[cjcenizal]

We can leverage Jest's diffing feature by tweaking this to return the actual request itself and then making assertions against that:

    it('should serialize the payload before sending it to Elasticsearch', async () => {
      // Return the payload we're sending to ES so we can make assertions against it.
      callWithRequestFactory.mockReturnValueOnce((_, payload = {}) => payload);

      const request = {
        params: { id: 'foo' },
        payload: {
          remoteCluster: 'bar1',
          leaderIndexPatterns: [ 'bar2' ],
          followIndexPattern: 'bar3',
        },
      };

      const response = await routeHandler(request);
      expect(response).toEqual({
        id: 'foo',
        body: {
          remote_cluster: 'bar1',
          leader_index_patterns: [ 'bar2' ],
          follow_index_pattern: 'bar3',
        },
      });
    });

This way I think the failures will be a little easier debug since they'll look something like this:

[sebelga]

Good point, not sure why I went in that direction in the first place 😊

[cjcenizal]

Can we add a bit more detail to the description so it stands in clear contrast against the failure state? E.g. "should create an auto-follow pattern when cluster is known".

[sebelga]

👍

[sebelga]

Thanks for the feedback! Yes, I didn't like that title there either but was not sure about the best solution for it and wanted to see with you. For phase 2, with the tabs (Follow index / Auto-follow pattern) the title might have more sense, we will see.
About the "Create" and "Add", I've been switching back and forth between those terms 😊 Let's go for "Create" then, and also in the code for our redux actions to be consistent.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request