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

add updatedAt to SimpleSavedObject #126359

Merged
merged 3 commits into from
Mar 9, 2022
Merged

add updatedAt to SimpleSavedObject #126359

merged 3 commits into from
Mar 9, 2022

Conversation

orouz
Copy link
Contributor

@orouz orouz commented Feb 24, 2022

  • add updated_at from SavedObject to SimpleSavedObject
  • update current SimpleSavedObject.updatedAt property after calling save()

(note: wasn't sure the PR template is relevant in this case, please LMK if i missed something)

@orouz orouz changed the title add updated_at field add updatedAt to SimpleSavedObject Feb 24, 2022
@orouz orouz marked this pull request as ready for review February 28, 2022 14:12
@orouz orouz requested a review from a team as a code owner February 28, 2022 14:12
@orouz orouz added release_note:skip Skip the PR/issue when compiling release notes v8.2.0 labels Feb 28, 2022
Comment on lines +87 to +90
.then((sso) => {
this.updatedAt = sso.updatedAt;
return sso;
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wasn't sure if this change warrants adding a test. thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that the current test suite is pretty lackluster, but let's not make the situation worse, and let's add tests on new enhancements if that's fine with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

happy to help. don't mind iterating too if needed 👍

@@ -58,6 +60,7 @@ export class SimpleSavedObject<T = unknown> {
this.migrationVersion = migrationVersion;
this.coreMigrationVersion = coreMigrationVersion;
this.namespaces = namespaces;
this.updatedAt = updatedAt;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed in order to show the value in a table column

Comment on lines 92 to 93
return this.client.create(this.type, this.attributes, {
migrationVersion: this.migrationVersion,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you also need to update updateAt when create-ing the object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed and included a test. woops!

Comment on lines +87 to +90
.then((sso) => {
this.updatedAt = sso.updatedAt;
return sso;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that the current test suite is pretty lackluster, but let's not make the situation worse, and let's add tests on new enhancements if that's fine with you.

@orouz orouz mentioned this pull request Mar 7, 2022
7 tasks
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
core 978 979 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 295.9KB 296.1KB +143.0B
Unknown metric groups

API count

id before after diff
core 2381 2382 +1

History

  • 💚 Build #26509 succeeded c1960c26c02dc4989d12a07fe5e102f0d07de39b
  • 💚 Build #26265 succeeded 1822bce8d10c0a6f9174a7ff33f6bc753c61f0f7
  • 💔 Build #26245 failed a96a16d25cfa172abce9ff7667e7d4a69b27bebc

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

@orouz orouz merged commit 06e453e into elastic:main Mar 9, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 9, 2022
…re-browser-errors

* 'main' of github.com:elastic/kibana: (46 commits)
  [Reporting] Capture Kibana stopped error (elastic#127017)
  add updatedAt to SimpleSavedObject (elastic#126359)
  Remove deprecated & unused `ElasticsearchServiceStart.legacy` (elastic#127050)
  remove opacity for fitting line series (elastic#127176)
  Remove deprecated & unused `HttpServiceSetup.auth` (elastic#127056)
  [Lens] Show underlying data editor navigation (elastic#125983)
  Bump dependencies (elastic#127238)
  Remove deprecated & unused `public-AsyncPlugin` (elastic#127048)
  Remove deprecated & unused `SavedObjectsImportFailure.title` (elastic#127043)
  skip flaky suite (elastic#123372)
  [kbn/generate] add basic package generator (elastic#127095)
  [build] Up compression quality (elastic#127064)
  Made fix to broken test. Deleted all existing pipelines before test starts. FLAKY: elastic#118593 (elastic#127102)
  Increase timeout for Jest integration tests (elastic#127220)
  skip failing test suite (elastic#126949)
  [DOCS] Adds note for data source performance impact (elastic#127184)
  [Security Solution] Adds CCS privileges warning enable switch in advanced settings (elastic#124459)
  [App Search] Move to tabbed single tabbed JSON flyout with upload and paste options and refactor cards (elastic#127162)
  Update dependency chromedriver to v99 (elastic#127079)
  [kbn/pm] add timings for more parts of bootstrap (elastic#127157)
  ...

# Conflicts:
#	x-pack/plugins/reporting/common/errors/index.ts
#	x-pack/plugins/reporting/server/lib/tasks/execute_report.ts
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 126359 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 11, 2022
@orouz orouz added the backport:skip This commit does not require backporting label Mar 14, 2022
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants