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

Rename image src to path and have src as the original value from the … #629

Merged
merged 4 commits into from
Sep 14, 2020

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Aug 27, 2020

…manifest

All the configs specified in the manifest file by the user should be exposed without changes in our JSON output. In the case of images, the src was overwritten by the path variable. This means what was in the manifest and outputted by JSON was not a 1:1 match.

It is ok to add additional fields in the JSON output but the original fields should never be touched.

This is a breaking change and need adjustements in Kibana.

@ruflin ruflin requested a review from ycombinator August 27, 2020 11:53
@ruflin ruflin self-assigned this Aug 27, 2020
@ruflin
Copy link
Contributor Author

ruflin commented Aug 27, 2020

@ycombinator This addresses the most urgent issue that we overwrite variables from the manifest. A follow up discussion is needed on what convention we follow for naming the different fields and standardise on from where the path is relative. I picked path for now as we use it the same way in other places too.

@ruflin
Copy link
Contributor Author

ruflin commented Aug 27, 2020

@skh @jfsiii I expect this would effect a few parts in Kibana to switch over to path from src for icons and screenshots.

@elasticmachine
Copy link

elasticmachine commented Aug 27, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #629 updated]

  • Start Time: 2020-09-14T09:48:02.862+0000

  • Duration: 7 min 17 sec

Test stats 🧪

Test Results
Failed 0
Passed 161
Skipped 0
Total 161

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM.

@ruflin
Copy link
Contributor Author

ruflin commented Aug 31, 2020

@jfsiii This is a breaking change for Kibana. Do you see a way to potentially support both options for a while. Something like if path does not exist, still use src and clean it up later.

@jfsiii
Copy link

jfsiii commented Sep 1, 2020

@ruflin yes, I believe that's right. I created elastic/kibana#76380 to track it. I'll leave for you & @ph to prioritize but it should ™️ be a quick & safe change

Could you add the new path property to the OpenAPI spec under https://github.com/ruflin/package-registry/blob/1e1106a48ab0fb309d245c90d3fcccdee4d2a06c/openapi.yml#L149-L150

@ruflin
Copy link
Contributor Author

ruflin commented Sep 3, 2020

I added the path in the openapi spec, thanks for the reminder.

For my own reference: Here the PR to test this with: elastic/kibana#76434

Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -107,6 +107,7 @@ type Owner struct {

type Image struct {
Src string `config:"src" json:"src" validate:"required"`
Path string `config:"path" json:"path"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change need a dependency update in integrations repository? Same question for package-storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so as this only changes the output generated by the registry and nothing on the package side itself. Anything specific you were thinking of?

Copy link
Contributor

@mtojek mtojek Sep 7, 2020

Choose a reason for hiding this comment

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

I was thinking about the case in which the Kibana expects the missing property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be solved by elastic/kibana#76434

…manifest

All the configs specified in the manifest file by the user should be exposed without changes in our JSON output. In the case of images, the src was overwritten by the path variable. This means what was in the manifest and outputted by JSON was not a 1:1 match.

It is ok to add additional fields in the JSON output but the original fields should never be touched.

This is a breaking change and need adjustements in Kibana.
@ruflin ruflin merged commit 4ee5642 into elastic:master Sep 14, 2020
@ruflin
Copy link
Contributor Author

ruflin commented Sep 14, 2020

Tested again with 7.10.0-SNAPSHOT of Kibana and seems to work as expected.

@ruflin ruflin deleted the fix-src-path branch September 14, 2020 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants