-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Serialize updated
value of entity document in response
#4646
Conversation
updated
value in the db out in the actionupdated
value in action doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only actions? All assets have an updated field.
common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
Outdated
Show resolved
Hide resolved
tests/src/test/scala/org/apache/openwhisk/core/controller/test/ActionsApiTests.scala
Outdated
Show resolved
Hide resolved
@rabbah Thank you for review. I think the updated field of activation doesn't need to be serialized because it has So I'll modify all assets (trigger, rules, and package) except |
235b568
to
e44dd6c
Compare
updated
value in action docupdated
value of entity document in response
13cf028
to
b09d783
Compare
Codecov Report
@@ Coverage Diff @@
## master #4646 +/- ##
==========================================
- Coverage 84.99% 78.32% -6.67%
==========================================
Files 198 198
Lines 8845 8837 -8
Branches 615 606 -9
==========================================
- Hits 7518 6922 -596
- Misses 1327 1915 +588
Continue to review full report at Codecov.
|
It passed all the tests 👍 |
common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
Show resolved
Hide resolved
@@ -48,7 +47,7 @@ abstract class WhiskEntity protected[entity] (en: EntityName, val entityType: St | |||
val version: SemVer | |||
val publish: Boolean | |||
val annotations: Parameters | |||
val updated = Instant.now(Clock.systemUTC()) | |||
val updated = WhiskEntity.currentMillis() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since all the subtypes now assign this value in the constructors, can we make this an abstract value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can't make this an abstract value because the WhiskActivation entity type doesn't assign this value to the constructor.
tests/src/test/scala/org/apache/openwhisk/core/controller/test/RulesApiTests.scala
Outdated
Show resolved
Hide resolved
Changes lgtm now with the added explanation. |
@@ -54,6 +54,10 @@ class PackagesApiTests extends ControllerTestCommon with WhiskPackagesApi { | |||
def aname() = MakeName.next("packages_tests") | |||
val parametersLimit = Parameters.sizeLimit | |||
|
|||
def checkResponse(response: WhiskPackage, expected: WhiskPackage) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kudos - thanks for doing this.
Looks like a failure in RulesApiTests |
One test case has been broken by refactoring. I just fixed it. |
3971288
to
1606a42
Compare
1606a42
to
2f8571b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
My comment would not block merging this.
@@ -61,6 +63,11 @@ class RulesApiTests extends ControllerTestCommon with WhiskRulesApi { | |||
val activeStatus = s"""{"status":"${Status.ACTIVE}"}""".parseJson.asJsObject | |||
val inactiveStatus = s"""{"status":"${Status.INACTIVE}"}""".parseJson.asJsObject | |||
val parametersLimit = Parameters.sizeLimit | |||
val dummyInstant = Instant.now() | |||
|
|||
def checkResponse(response: WhiskRuleResponse, expected: WhiskRuleResponse) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small nit can be extracting this kind of methods into a trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@style95 Thank you for review, I've created a common method to test the WhiskAction, WhiskActionMetaData, WhiskTrigger, and WhiskPackage. But I couldn't extract the method in the RulesApiTests because WhiskRuleResponse doesn't extend WhiskEntity.
Please review it again 😄
Description
Since an action API doesn't serialize the
updated
value of the action entity, users can't know when the action was updated.It changes the action API to serialize and provide the
updated
value of the actionRelated issue and scope
#4228
My changes affect the following components
Types of changes
Checklist: