[ServiceNow] Support different date, time and datetime formats #88
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
src/main/java/io/cdap/plugin/servicenow/connector/ServiceNowRecordConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/connector/ServiceNowRecordConverter.java
Outdated
Show resolved
Hide resolved
| .appendOptional(DateTimeFormatter.ofPattern("HH:mm:ss")) | ||
| .appendOptional(DateTimeFormatter.ofPattern("HH:mm")) |
There was a problem hiding this comment.
unable to find this in reference link pasted above, can you please point me to the doc where these are mentioned?
There was a problem hiding this comment.
there is no direct reference to time format in the link, so used the time patterns from the datetime patterns.
There was a problem hiding this comment.
why not hh:mm:ss a && hh.mm.ss a?
There was a problem hiding this comment.
Also are all of these tested? We should not do guess work in prod plugins.
There was a problem hiding this comment.
Regarding support for AM/PM those are majorly used with datetime and not time, hence didn't add it.
Regarding the testing, most of them are tested using the unit test and one of the failure pattern was verified by the customer runs.
There was a problem hiding this comment.
unit testing is one thing but how do we know if they are actually supported by service now if we have added them based on assumption from datetime patterns. Did we do an e2e test with the actual service now data?
There was a problem hiding this comment.
removed non-redundant time patterns as confirmed with Servicenow UI, the removed patterns are not required
itsankit-google
left a comment
itsankit-google
left a comment
There was a problem hiding this comment.
I don't see e2e tests for added time formats.
Also I don't see any e2e tests running:
|
e2e tests are being enabled for servicenow in #89. |
We need to enable it first, run against this PR. Then only adding any e2e tests make sense. |
src/main/java/io/cdap/plugin/servicenow/connector/ServiceNowRecordConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/connector/ServiceNowRecordConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/connector/ServiceNowRecordConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/connector/ServiceNowRecordConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/connector/ServiceNowRecordConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/connector/ServiceNowRecordConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/connector/ServiceNowRecordConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/connector/ServiceNowRecordConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/connector/ServiceNowRecordConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/connector/ServiceNowRecordConverter.java
Outdated
Show resolved
Hide resolved
itsankit-google
left a comment
itsankit-google
left a comment
There was a problem hiding this comment.
Fix e2e test failures.
|
e2e tests are being added in https://github.com/data-integrations/servicenow-plugins/pull/92/files |
…dle the datetime parsing issue.
prince-cs
force-pushed
the
bugfix/support-different-datetime-formats
branch
from
14b9c2f to
8dc6b5e
Compare
3 participants
Support different date, time and datetime formats to handle the ServiceNow datetime parsing issue.
Reference date & time formats: https://www.servicenow.com/docs/bundle/washingtondc-api-reference/page/app-store/dev_portal/API_reference/GlideDateTime/concept/c_GlideDateTimeAPI.html