Custom event implementation #51
Conversation
Custom event implementation
There was a problem hiding this comment.
A couple of more comments:
- To confirm, does this PR cover the support of custom event for export, lint, init, and it does not cover import, right?
- I don't see the command line help info is updated to reflect the latest changes introduced by the support of custom event.
- Next time, we can split the code change into multiple PRs, and each one includes one functional modification, e.g.: one PR for export, one PR for init, and so on, which is easier for code review.
| } | ||
|
|
||
| readmeContent += ` | ||
| ## Installation and Usage |
There was a problem hiding this comment.
The existing packages may need to be updated, e.g.: the Installation and Usage section seems to be the 3 level title, now has been moved up to 2 level.
tools/integration/src/index.ts
Outdated
| const url = `https://${server}/api/custom-dashboard` | ||
| logger.info(`Start to get the dashboard list from ${url}`); | ||
| // Event export | ||
| const eventId = getValueByKeyFromArray(eventIncludes, "eventId"); |
There was a problem hiding this comment.
Shall we consistently use id instead of eventId as what we do with dashboards, since the type event has been specified by type=event
tools/integration/src/index.ts
Outdated
| try { | ||
| const filename = `${title}.json`; | ||
| const filepath = path.join(dir, filename); | ||
| logger.info(`Saving dashboard (id=${id}) to ${filepath}`); |
There was a problem hiding this comment.
When log a message, usually when it says doing something, in this case, saving, we will append ... at the end of the log line. The same applies to all the other log messages through out this file.
tools/integration/src/index.ts
Outdated
| const filepath = path.join(dir, filename); | ||
| logger.info(`Saving event (id=${id}) to ${filepath}`); | ||
| fs.writeFileSync(filepath, JSON.stringify(event, null, 2)); | ||
| logger.info(`Event (id=${id}) saved successfully`); |
There was a problem hiding this comment.
Please be sure that we use the term consistently w/o ambiguity when it comes to the event manipulation. For example, here we say some event is saved successfully. This may be confusing to the end user, because it's actually a custom event definition, not an event instance. We probably should use event definition through out this file.
Dashboard might have the same problem.
Publish Logging
Publish Logging Testing
Publish pipeline testing
Testing publish logging
| ${execName} export --server example.com --include 'type=dashboard title="Runtime.*"' --location ./my-package | ||
| ${execName} export --server example.com --include 'type=event id=exampleid' --location ./my-package |
There was a problem hiding this comment.
Are the single quotes required? I'd expect this is not needed. If it's not true, we can solve it in a follow-up PR.
There was a problem hiding this comment.
The single quotes aren't strictly necessary. We'll address this in a follow-up PR.
| logger.info(`Applying the dashboard to ${url} ...`); | ||
| const url = `https://${server}/${apiPath}`; | ||
|
|
||
| function getTypeLabel(apiPath: String): String { |
There was a problem hiding this comment.
You don't have to dynamically determine the type by apiPath, because every time when you call importIntegration, you know the type, you can pass it into importIntegration as a param.
| if (matches.length && inc.explicitlyTyped) { | ||
| filtered = matches.map(idCond => { | ||
| const id = idCond.split("=")[1]?.replace(/^"|"$/g, ''); | ||
| return { id }; | ||
| }); | ||
| } else if (matches.length) { | ||
| filtered = matches.map(idCond => { | ||
| const id = idCond.split("=")[1]?.replace(/^"|"$/g, ''); | ||
| return allDashboards.find(d => d.id === id); | ||
| }).filter(Boolean); | ||
| } else { |
There was a problem hiding this comment.
What are the difference between the two blocks? Why it doesn't have to check the existence of the id in allDashboards in the first if block?
| const isOnlyDashboard = parsedIncludes.every(inc => inc.type === "dashboard"); | ||
| const isOnlyEvent = parsedIncludes.every(inc => inc.type === "event"); | ||
| const isMixedOrUnspecified = parsedIncludes.some(inc => inc.type === "all") || (!isOnlyDashboard && !isOnlyEvent); | ||
|
|
||
| if (!wasDashboardFound && !wasEventFound && isMixedOrUnspecified) { | ||
| logger.error("No custom elements were found or exported."); | ||
| } |
There was a problem hiding this comment.
The code logic here is tricky. Shall we just simply count the processed dashboards and events, and if there's nothing processed, we output no custom elements exported. We should not rely on the presence of type param in the --include clause, because it could be possible that no element is processed even the type is specified, e.g.: no match found for this type.
…vability-as-code into test-environment
Custom event implementation
No description provided.