[SR] Support capitalized date formats in snapshot names #53751

jkelastic · 2019-12-22T01:31:10Z

Release note

Snapshot names that contain date math may require capital letters, e.g. "<snapshot-{now/d{yyyy.MM.dd|+09:00}}>". This change fixes a bug in which the UI rejected snapshot names containing capital letters by scoping this validation to only the name part of this pattern, ignoring the date math part.

Summary

Fixed 51857

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-12-22T01:31:12Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

jkelastic · 2019-12-22T01:42:22Z

It looks the top three commits with X are from another feature fix and included in this branch, was I suppose to clear the git log or previous commits?

kibanamachine · 2019-12-22T02:45:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dce230cedcaa2bc30a38609457e33da9620e9884

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

jkelastic · 2019-12-27T22:25:45Z

@elasticmachine merge upstream

kibanamachine · 2019-12-27T23:45:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3ec83c0

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

jloleysens · 2019-12-30T13:42:37Z

...k/legacy/plugins/snapshot_restore/public/app/components/policy_form/steps/step_logistics.tsx

@@ -347,7 +347,7 @@ export const PolicyStepLogistics: React.FunctionComponent<StepProps> = ({
          onChange={e => {
            updatePolicy(
              {
-                snapshotName: e.target.value.toLowerCase(),
+                snapshotName: e.target.value,


@jkelastic I think we are on the right track with the proposed changes, I would just like to know what the reason was that we .toLowerCase()ed originally? Did we just never need to take this extra step?

@jloleysens yes, we never need to take this step as .toLowerCase() on date format will negate MM to mm. The uppercase matters as MM is month and mm is minutes.

@jkelastic see @alisonelizabeth 's comment below, it looks like we need to keep the names lowercase :)

…ield

jkelastic · 2019-12-30T17:30:35Z

@elasticmachine merge upstream

jkelastic · 2020-01-02T18:07:15Z

@elasticmachine merge upstream

alisonelizabeth

@jkelastic nice job with this! Tested locally and verified fix.

In addition to this change, I think it would be great if we could be a little more robust with the client validation. A snapshot name must be lowercase, however, the date math expression should be able to accept upper and lowercase letters.

For example, with your current change, I can create a snapshot name in the UI like so: MY_SNAPSHOT or <SNAPSHOT-{now/d{yyyy.MM.dd|+09:00}}>, but I get an error back from ES indicating that the name must be lowercase. I think it would be ideal if we could check for this on the client, before the user tries to submit the form.

…into bugfix/Snapshot_restore

cjcenizal · 2020-01-07T17:20:38Z

@elasticmachine merge upstream

…c/kibana into bugfix/Snapshot_restore

…rwise

jkelastic · 2020-01-10T05:48:40Z

@elasticmachine merge upstream

jloleysens

It looks like changes from #53757 snuck in here :(

This is the offending commit: 3c5ffaa

One thing we can do is to re-target this PR to your watcher PR and merge watcher first then this (once both are approved). Alternatively you can checkout one commit before ☝️ and cherry-pick the changes intended for SR. Up to you!

Actually, I'd recommend we just wait until #53757 is merged first.

jloleysens · 2020-01-10T09:44:46Z

x-pack/legacy/plugins/snapshot_restore/public/app/services/validation/validate_policy.ts

+const isSnapshotNameLowerCase = (str: string): boolean => {
+  const strExcludeDate =
+    str.substring(0, str.search('{')) + str.substring(str.search('}}') + 2, str.length);
+  return strExcludeDate !== strExcludeDate.toLowerCase() ? true : false;


Can be simplified to:

Suggested change

return strExcludeDate !== strExcludeDate.toLowerCase() ? true : false;

return strExcludeDate !== strExcludeDate.toLowerCase();

Additionally, given the boolean check this function name (isSnapshotNameLowerCase) is misleading. Either the name should be isSnapshotNameNotLowerCase or we should change !== to === and update the if logic on L70.

Additionally, given the boolean check this function name (isSnapshotNameLowerCase) is misleading. Either the name should be isSnapshotNameNotLowerCase or we should change !== to === and update the if logic on L70.

Thanks Jean for the advice. I've changed the name to isSnapshotNameNotLowerCase and simplified the return strExcludeDate condition

jloleysens · 2020-01-10T14:57:22Z

x-pack/legacy/plugins/snapshot_restore/public/app/services/validation/validate_policy.ts

@@ -15,6 +15,12 @@ const isStringEmpty = (str: string | null): boolean => {
  return str ? !Boolean(str.trim()) : true;
 };

+const isSnapshotNameLowerCase = (str: string): boolean => {
+  const strExcludeDate =
+    str.substring(0, str.search('{')) + str.substring(str.search('}}') + 2, str.length);


Is the double }} intended? At the moment the validation only passes when I add a trailing }. Resulting in:

A comment explaining this logic would also be great :) - something about the what the string may contain or is expected to look like.

Is the double }} intended? At the moment the validation only passes when I add a trailing }. Resulting in:

The }} was intended. However, I think I've over analyzed date format syntax and should just check for } because all date format will have one { and } and not all have }}

A comment explaining this logic would also be great :) - something about the what the string may contain or is expected to look like.

Will do, I will put it in the code

…k for { and }>

…c/kibana into bugfix/Snapshot_restore

jkelastic · 2020-01-10T18:42:48Z

@elasticmachine merge upstream

jloleysens

I'm happy for this to be merged if you've tested that with the current validation we can create snapshot policies 👍.

alisonelizabeth · 2020-01-13T16:39:24Z

@elasticmachine merge upstream

kibanamachine · 2020-01-13T19:13:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 41e5358

History

💚 Build #19814 succeeded 41e5358
💚 Build #19411 succeeded 6839756
💔 Build #19382 failed d6a2102
💔 Build #19345 failed ef626bc
💚 Build #19188 succeeded 58d763a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

) Snapshot names that contain date math may require capital letters, e.g. "<snapshot-{now/d{yyyy.MM.dd|+09:00}}>". This change fixes a bug which complained that capital letters are not allowed in snapshot names, by scoping this validation to only the name part of this pattern, ignoring the date math part. Co-authored-by: Jimmy Kuang <[email protected]>

jkelastic added release_note:fix v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI v7.6.0 labels Dec 22, 2019

Removed toLowerCase() from step_logistics.tsx line 350

0fa149b

jkelastic force-pushed the bugfix/Snapshot_restore branch from dce230c to 0fa149b Compare December 27, 2019 22:23

elasticmachine and others added 4 commits December 27, 2019 14:25

Merge branch 'master' into bugfix/Snapshot_restore

3ec83c0

Adding Scheme field to Webhook

27092fe

Remove debugger breakpoint

d602d4d

Remove debugger tag

f3652ac

jloleysens reviewed Dec 30, 2019

View reviewed changes

jkelastic added 2 commits December 30, 2019 09:18

Fix undefined path variable and moved scheme field in front of host f…

8601dd6

…ield

Fixed Elint error

2b0a12b

elasticmachine added 2 commits December 30, 2019 09:30

Merge branch 'master' into bugfix/Snapshot_restore

62647d8

Merge branch 'master' into bugfix/watcher

722b74f

Merge branch 'master' into bugfix/Snapshot_restore

2e4f540

alisonelizabeth reviewed Jan 2, 2020

View reviewed changes

Merge branch 'bugfix/watcher' of https://github.com/jkelastic/kibana …

3c5ffaa

…into bugfix/Snapshot_restore

elasticmachine and others added 3 commits January 7, 2020 09:20

Merge branch 'master' into bugfix/Snapshot_restore

46b2358

Merge branch 'bugfix/Snapshot_restore' of https://github.com/jkelasti…

9adccdd

…c/kibana into bugfix/Snapshot_restore

Add lower casing check to Snapshot Name L69 in validate_policy.ts

af481e9

jkelastic added 2 commits January 9, 2020 18:22

Correct condition to ignore casing for date format and error out othe…

f1df9ec

…rwise

Removed jest case I've introduce but was unnecessary

5a01e5f

Merge branch 'master' into bugfix/Snapshot_restore

58d763a

jloleysens requested changes Jan 10, 2020

View reviewed changes

jloleysens reviewed Jan 10, 2020

View reviewed changes

jkelastic added 3 commits January 10, 2020 08:43

Rename to isSnapshotNameNotLowerCase and updated if condition to chec…

6a6e92d

…k for { and }>

Merge branch 'bugfix/Snapshot_restore' of https://github.com/jkelasti…

0ed7f41

…c/kibana into bugfix/Snapshot_restore

Merge branch 'master' into bugfix/Snapshot_restore

ef626bc

elasticmachine and others added 2 commits January 10, 2020 12:42

Merge branch 'master' into bugfix/Snapshot_restore

d6a2102

Fix eslint errors.

6839756

jloleysens approved these changes Jan 10, 2020

View reviewed changes

Merge branch 'master' into bugfix/Snapshot_restore

41e5358

cjcenizal changed the title ~~[SR] Snapshot name forced to lowercase~~ [SR] Support capitalized date formats in snapshot names Jan 13, 2020

cjcenizal merged commit 79ee978 into elastic:master Jan 13, 2020

cjcenizal mentioned this pull request Jan 13, 2020

[7.x] [SR] Support capitalized date formats in snapshot names (#53751) #54610

Merged

cjcenizal mentioned this pull request Jan 14, 2020

[SLM] Kibana UI forces unwanted lower casing for snapshot name #54795

Closed

cjcenizal added v7.5.2 and removed v7.5.2 labels Jan 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SR] Support capitalized date formats in snapshot names #53751

[SR] Support capitalized date formats in snapshot names #53751

jkelastic commented Dec 22, 2019 •

edited by cjcenizal

Loading

elasticmachine commented Dec 22, 2019

jkelastic commented Dec 22, 2019

kibanamachine commented Dec 22, 2019

jkelastic commented Dec 27, 2019

kibanamachine commented Dec 27, 2019

jloleysens Dec 30, 2019

jkelastic Dec 30, 2019

jloleysens Jan 7, 2020

jkelastic commented Dec 30, 2019

jkelastic commented Jan 2, 2020

alisonelizabeth left a comment •

edited

Loading

cjcenizal commented Jan 7, 2020

jkelastic commented Jan 10, 2020

jloleysens left a comment •

edited

Loading

jloleysens Jan 10, 2020

jloleysens Jan 10, 2020 •

edited

Loading

jkelastic Jan 10, 2020

jloleysens Jan 10, 2020

jloleysens Jan 10, 2020 •

edited

Loading

jkelastic Jan 10, 2020

jkelastic commented Jan 10, 2020

jloleysens left a comment

alisonelizabeth commented Jan 13, 2020

kibanamachine commented Jan 13, 2020

	return strExcludeDate !== strExcludeDate.toLowerCase() ? true : false;
	return strExcludeDate !== strExcludeDate.toLowerCase();

[SR] Support capitalized date formats in snapshot names #53751

[SR] Support capitalized date formats in snapshot names #53751

Conversation

jkelastic commented Dec 22, 2019 • edited by cjcenizal Loading

Release note

Summary

Checklist

For maintainers

elasticmachine commented Dec 22, 2019

jkelastic commented Dec 22, 2019

kibanamachine commented Dec 22, 2019

💚 Build Succeeded

jkelastic commented Dec 27, 2019

kibanamachine commented Dec 27, 2019

💚 Build Succeeded

jloleysens Dec 30, 2019

Choose a reason for hiding this comment

jkelastic Dec 30, 2019

Choose a reason for hiding this comment

jloleysens Jan 7, 2020

Choose a reason for hiding this comment

jkelastic commented Dec 30, 2019

jkelastic commented Jan 2, 2020

alisonelizabeth left a comment • edited Loading

Choose a reason for hiding this comment

cjcenizal commented Jan 7, 2020

jkelastic commented Jan 10, 2020

jloleysens left a comment • edited Loading

Choose a reason for hiding this comment

jloleysens Jan 10, 2020

Choose a reason for hiding this comment

jloleysens Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

jkelastic Jan 10, 2020

Choose a reason for hiding this comment

jloleysens Jan 10, 2020

Choose a reason for hiding this comment

jloleysens Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

jkelastic Jan 10, 2020

Choose a reason for hiding this comment

jkelastic commented Jan 10, 2020

jloleysens left a comment

Choose a reason for hiding this comment

alisonelizabeth commented Jan 13, 2020

kibanamachine commented Jan 13, 2020

💚 Build Succeeded

History

jkelastic commented Dec 22, 2019 •

edited by cjcenizal

Loading

alisonelizabeth left a comment •

edited

Loading

jloleysens left a comment •

edited

Loading

jloleysens Jan 10, 2020 •

edited

Loading

jloleysens Jan 10, 2020 •

edited

Loading