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

🔭 [Frontend/ProductCatalogService] Updated Product Catalog Service Data to Match Astronomy Store #285

Merged
merged 11 commits into from
Aug 19, 2022

Conversation

xoscar
Copy link
Contributor

@xoscar xoscar commented Aug 10, 2022

Fixes #282.

Changes

Updates the product service catalog data to have products that match the OTEL astronomy theme.

https://www.loom.com/share/828a3e2e6b1e4802894abeb3deda9464

@xoscar xoscar marked this pull request as ready for review August 10, 2022 15:56
@xoscar xoscar requested a review from a team August 10, 2022 15:56
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@mic-max mic-max left a comment

Choose a reason for hiding this comment

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

@xoscar Could you run the local service tests to make sure nothing broke there /test/
Also, good idea to keep the existing ID
I agree with @reyang though, we should source all free to use images

@xoscar
Copy link
Contributor Author

xoscar commented Aug 11, 2022

Hey @mic-max & @reyang thanks for your review. I'm getting the non-copyrighted images and I'll be updating the PR.

For the /tests/ I found out all of the tests are failing because of this change 55d9919 I think we should run the tests within the containers context so we don't have to expose the ports to localhost.

@mic-max
Copy link
Contributor

mic-max commented Aug 11, 2022

context so we don't have to expose the ports to localhost
Ah yes, I forgot about that PR. I will work on getting those tests back up and running :)

@xoscar
Copy link
Contributor Author

xoscar commented Aug 11, 2022

@mic-max & @reyang the PR should be ready for a second review 😬

@xoscar
Copy link
Contributor Author

xoscar commented Aug 15, 2022

Hey @mic-max & @reyang let me know if there is anything else I can do. The PR should be ready for its final review 😄

Copy link
Contributor

@mic-max mic-max left a comment

Choose a reason for hiding this comment

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

Looks great!

@cartersocha
Copy link
Contributor

Could we get a changelog entry and potentially readme guidance on how to add more items? The readme update can potentially follow in another pr

@cartersocha
Copy link
Contributor

We should agree on some sort of file naming conventions to follow too. Our docs are word_word and pictures are word-word. These new images are WordWord.

@cartersocha
Copy link
Contributor

I'm fine with pictures all one way and markdown files the other but we need some sort of consistency here

@puckpuck
Copy link
Contributor

Did we address @reyang's concern about ensuring all images are not copyrighted? If so, we can resolve that conversation and merge this.

@xoscar
Copy link
Contributor Author

xoscar commented Aug 18, 2022

I just added a changelog entry and I think it should be ready to be merged. @cartersocha I'll add the steps to add a new product on a separate PR if you don't mind. CC: @puckpuck

@xoscar
Copy link
Contributor Author

xoscar commented Aug 18, 2022

@cartersocha the copyright issue has been solved as well

@cartersocha
Copy link
Contributor

@xoscar update & we can merge!

@xoscar
Copy link
Contributor Author

xoscar commented Aug 19, 2022

@cartersocha done 😄

@cartersocha cartersocha merged commit f948dcd into open-telemetry:main Aug 19, 2022
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
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.

[ProductCatalogService/Frontend] Update Product Service Catalog Data to Match Astrology store
5 participants