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

feat(ui): Host platform images on datahub-web-react #4118

Merged
merged 3 commits into from
Feb 16, 2022

Conversation

ngamanda
Copy link
Contributor

@ngamanda ngamanda commented Feb 11, 2022

Adds capability for users to host their platform images (from src/images) on their react app instead.

Context: https://datahubspace.slack.com/archives/CV2UXSE9L/p1644477881072309

Changes

  • Installs CopyWebpackPlugin to copy src/images to platforms
  • [Updated] data_platforms.json files are updated to use the new relative url

Sample Usage:

// data_platforms.json
{
    "urn": "urn:li:dataPlatform:hive",
    "aspect": {
      "datasetNameDelimiter": ".",
      "name": "hive",
      "displayName": "Hive",
      "type": "FILE_SYSTEM",
      - "logoUrl": "https://raw.githubusercontent.com/linkedin/datahub/master/datahub-web-react/src/images/hivelogo.png"
      + "logoUrl": "/assets/platforms/hivelogo.png"
    }
  },

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

@github-actions
Copy link

github-actions bot commented Feb 11, 2022

Unit Test Results (build & test)

  70 files  +1    70 suites  +1   15m 49s ⏱️ + 1m 2s
609 tests +5  550 ✔️ +5  59 💤 ±0  0 ±0 

Results for commit bedc690. ± Comparison against base commit 9bdc9af.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@gabe-lyons gabe-lyons left a comment

Choose a reason for hiding this comment

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

Hey @ngamanda, thoughts on just doing this for everyone? I think most (if not all) of folks would prefer hosting their own platform logos, and the additional cost should be pretty small. Any reason not to automatically do this?

@ngamanda
Copy link
Contributor Author

Hi @gabe-lyons ! I thought to leave it as optional. But yes, I'd think most users would also prefer to host their own assets. I've updated the PR with the changes :)

Comment on lines 17 to 25
webpack: {
plugins: {
add: [
new CopyWebpackPlugin({
patterns: [{ from: 'src/images', to: 'platforms' }],
}),
],
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just add an explanation for why the copy is needed? otherwise looks great!! thanks @ngamanda

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

@github-actions
Copy link

Unit Test Results (metadata ingestion)

    3 files  ±  0      3 suites  ±0   41m 22s ⏱️ - 1m 40s
317 tests +  5  317 ✔️ +  5    0 💤 ±0  0 ±0 
908 runs  +12  886 ✔️ +10  22 💤 +2  0 ±0 

Results for commit bedc690. ± Comparison against base commit 9bdc9af.

This pull request removes 1 and adds 6 tests. Note that renamed tests count towards both.
tests.unit.test_utilities ‑ test_groupby_unsorted
tests.integration.kafka.test_kafka_state ‑ test_kafka_ingest_with_stateful
tests.unit.stateful_ingestion.test_kafka_state ‑ test_kafka_common_state
tests.unit.test_athena_source ‑ test_athena_get_table_properties
tests.unit.test_athena_source ‑ test_athena_uri
tests.unit.test_kafka_source.KafkaSourceTest ‑ test_kafka_source_stateful_ingestion_requires_platform_instance
tests.unit.test_kafka_source.KafkaSourceTest ‑ test_kafka_source_workunits_with_platform_instance

Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

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

LGTM!

@shirshanka shirshanka merged commit d17f2bf into datahub-project:master Feb 16, 2022
hevandro-veiga pushed a commit to hevandro-veiga/datahub that referenced this pull request Feb 18, 2022
…4118)

* feat(react-images): host platform images on datahub-web-react

* feat(react-images): update data_platforms logoUrl

* feat(react-images): add explanation for CopyWebpackPlugin

Co-authored-by: Amanda Ng <[email protected]>
maggiehays pushed a commit to maggiehays/datahub that referenced this pull request Aug 1, 2022
…4118)

* feat(react-images): host platform images on datahub-web-react

* feat(react-images): update data_platforms logoUrl

* feat(react-images): add explanation for CopyWebpackPlugin

Co-authored-by: Amanda Ng <[email protected]>
@ngamanda ngamanda deleted the fix-image-hosting branch October 3, 2022 09:40
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.

3 participants