chore(certs): Remove certificate for old data-accelerator domain#161
chore(certs): Remove certificate for old data-accelerator domain#161martyngigg wants to merge 9 commits into
Conversation
Remove old config values.
If they are important the commit date can be used.
WalkthroughThis pull request updates documentation and infrastructure configuration for the Analytics Data Platform. Changes include refactored deployment procedures, simplified floating IP requirements, new architecture diagrams, removal of legacy "accelerator" references from Traefik configuration, ADR metadata cleanup, and a new Lakekeeper adoption ADR. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
docs-devel/src/index.md (1)
16-20: Consider minor wording refinement for conciseness.Line 18 could be slightly more concise. Change "containing all of the code for the platform" to "containing all the code for the platform" (removing "of" improves readability).
docs-devel/src/system-architecture/adrs/0006-use-lakekeeper-as-iceberg-catalog.md (3)
11-11: Add missing comma in list.Line 11 needs a comma before "including:" for proper punctuation.
Apply this diff:
-Several implementations exist within the Iceberg libraries including: +Several implementations exist within the Iceberg libraries, including:
24-24: Add missing comma in clause.Line 24 needs a comma to connect two independent clauses.
Apply this diff:
-The REST specification offers greater flexibility in how clients interact with the catalog and several +The REST specification offers greater flexibility in how clients interact with the catalog, and several
49-51: Fix compound hyphenation and punctuation.Line 49 contains two issues: "back bone" should be one word "backbone", and a comma is needed before "so" to properly separate independent clauses. Line 51 needs a comma before "as".
Apply this diff:
-Lakekeeper will be the back bone of the data lake and is a relatively new project so there is some -risk that it does not survive. -The design of Apache Iceberg mitigates the risk of migrating to a new catalog if that becomes -necessary as the catalog only stores metadata and would not require migrating the data on the object +Lakekeeper will be the backbone of the data lake, and it is a relatively new project, so there is some +risk that it does not survive. +The design of Apache Iceberg mitigates the risk of migrating to a new catalog if that becomes +necessary, as the catalog only stores metadata and would not require migrating the data on the objectdocs-devel/src/deployment/index.md (2)
49-49: Consider adding a comma for readability.The sentence would read more clearly with a comma: "Once deployed, check the Traefik dashboard is available at..."
Apply this diff:
-Once deployed check the Traefik dashboard is available at `https://<domain>/traefik/dashboard/.` +Once deployed, check the Traefik dashboard is available at `https://<domain>/traefik/dashboard/.`
55-55: Consider simplifying the phrasing.The phrase "with the exception of" could be simplified to "except for" or "except" for better readability.
Apply this diff:
-services depend on others being available. Each service has a single VM with the exception of Superset that hosts multiple instances on one machine. +services depend on others being available. Each service has a single VM except for Superset that hosts multiple instances on one machine.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (2)
docs-devel/src/assets/images/analytics-data-platform-arch.pngis excluded by!**/*.pngdocs-devel/src/assets/images/lakehouse-poc.pngis excluded by!**/*.png
📒 Files selected for processing (19)
docs-devel/mkdocs.yml(1 hunks)docs-devel/src/assets/images/.gitignore(1 hunks)docs-devel/src/assets/images/analytics-data-platform-arch.drawio(1 hunks)docs-devel/src/assets/images/lakehouse-poc.drawio(0 hunks)docs-devel/src/deployment/index.md(1 hunks)docs-devel/src/deployment/prerequisites.md(1 hunks)docs-devel/src/index.md(2 hunks)docs-devel/src/system-architecture/adrs/0001-record-architecture-decisions.md(0 hunks)docs-devel/src/system-architecture/adrs/0002-use-ansible-for-vm-provisioning-and-configuration.md(0 hunks)docs-devel/src/system-architecture/adrs/0003-use-scd-cloud-vms.md(0 hunks)docs-devel/src/system-architecture/adrs/0004-use-minio-as-object-store.md(0 hunks)docs-devel/src/system-architecture/adrs/0005-use-apache-iceberg-as-lakehouse-table-format.md(0 hunks)docs-devel/src/system-architecture/adrs/0006-use-lakekeeper-as-iceberg-catalog.md(1 hunks)docs-devel/src/system-architecture/index.md(1 hunks)infra/ansible-docker/group_vars/all/openstack.yml(0 hunks)infra/ansible-docker/playbooks/cloud/private_network_create.yml(1 hunks)infra/ansible-docker/readme.md(1 hunks)infra/ansible-docker/roles/traefik/tasks/main.yml(0 hunks)infra/ansible-docker/roles/traefik/templates/etc/traefik/dynamic/tls.yml.j2(0 hunks)
💤 Files with no reviewable changes (9)
- docs-devel/src/system-architecture/adrs/0004-use-minio-as-object-store.md
- infra/ansible-docker/roles/traefik/templates/etc/traefik/dynamic/tls.yml.j2
- docs-devel/src/system-architecture/adrs/0005-use-apache-iceberg-as-lakehouse-table-format.md
- infra/ansible-docker/roles/traefik/tasks/main.yml
- docs-devel/src/system-architecture/adrs/0002-use-ansible-for-vm-provisioning-and-configuration.md
- docs-devel/src/system-architecture/adrs/0001-record-architecture-decisions.md
- docs-devel/src/system-architecture/adrs/0003-use-scd-cloud-vms.md
- infra/ansible-docker/group_vars/all/openstack.yml
- docs-devel/src/assets/images/lakehouse-poc.drawio
🧰 Additional context used
🪛 LanguageTool
docs-devel/src/index.md
[style] ~18-~18: Consider removing “of” to be more concise
Context: ...his repository is a monorepo containing all of the code for the platform. It may be separa...
(ALL_OF_THE)
docs-devel/src/deployment/index.md
[uncategorized] ~49-~49: Possible missing comma found.
Context: ... playbooks/traefik/deploy.yml ``` Once deployed check the Traefik dashboard is availabl...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~55-~55: Possible missing comma found.
Context: ...ng available. Each service has a single VM with the exception of Superset that hos...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~55-~55: Consider using “except” or “except for”
Context: ...available. Each service has a single VM with the exception of Superset that hosts multiple instances ...
(WITH_THE_EXCEPTION_OF)
[uncategorized] ~56-~56: Possible missing preposition found.
Context: ... Superset that hosts multiple instances one once machine. First create the VMs: `...
(AI_HYDRA_LEO_MISSING_IN)
docs-devel/src/system-architecture/adrs/0006-use-lakekeeper-as-iceberg-catalog.md
[uncategorized] ~11-~11: Possible missing comma found.
Context: ...mplementations exist within the Iceberg libraries including: - REST: a server-side catal...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...bility in how clients interact with the catalog and several implementations exist that ...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~49-~49: This is normally spelled as one word.
Context: ...## Consequences Lakekeeper will be the back bone of the data lake and is a relatively ne...
(BACK_COMPOUNDS)
[uncategorized] ~49-~49: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ata lake and is a relatively new project so there is some risk that it does not sur...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~51-~51: Possible missing comma found.
Context: ...rating to a new catalog if that becomes necessary as the catalog only stores metadata and...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (13)
docs-devel/src/assets/images/.gitignore (1)
1-2: Well-structured gitignore entry.The pattern and comment clearly document the purpose. The temporary Draw.io save pattern is appropriate for ignoring editor artefacts.
infra/ansible-docker/playbooks/cloud/private_network_create.yml (1)
2-8: Variable loading strategy is sound.The new play ensures global configuration is loaded before network provisioning tasks execute. The use of
ansible.builtin.include_varsis the correct approach for loading variables from group_vars.docs-devel/src/deployment/prerequisites.md (1)
78-85: Simplified networking approach is well documented.The updated guidance correctly reflects the single floating IP requirement. The DNS-based validation approach is more robust than checking variable values directly.
docs-devel/mkdocs.yml (1)
3-3: Domain migration correctly applied.The site URL has been updated from the legacy data-accelerator domain to the new analytics domain. The path structure also reflects a more consolidated approach.
docs-devel/src/index.md (1)
1-40: Comprehensive documentation improvements.The updated title and new repository overview sections provide valuable context. The Details section with links improves navigation. Repository structure documentation is particularly helpful for new contributors.
docs-devel/src/system-architecture/index.md (1)
3-8: Improved architecture documentation structure.The introductory text and deployment link provide better context for readers. The updated image reference aligns with the new analytics-data-platform architecture diagram.
infra/ansible-docker/readme.md (1)
1-5: README effectively delegates to primary documentation.The simplified approach is appropriate, referring readers to the comprehensive deployment documentation. The title and description clearly define the module's purpose.
docs-devel/src/system-architecture/adrs/0006-use-lakekeeper-as-iceberg-catalog.md (1)
1-20: Thorough ADR with clear rationale.The decision context effectively compares Iceberg catalog implementations. The comparison of Gravitino, Polaris, Nessie, and Lakekeeper provides good documentation of the evaluation process.
docs-devel/src/assets/images/analytics-data-platform-arch.drawio (1)
1-163: LGTM! Architecture diagram added successfully.The Draw.io diagram file has been added with valid XML structure and represents the Analytics Data Platform architecture comprehensively, including key components such as Superset, Trino, Keycloak, Lakekeeper, and various data sources and flows.
docs-devel/src/deployment/index.md (4)
7-10: LGTM! Good addition of deployment caveats.The note clarifying that the system is not yet production-grade sets appropriate expectations for users following the deployment guide.
14-16: LGTM! Path updates are clear.The updated paths to
ansible-docker/playbooksand explicit working directory statement improve clarity for deployment operators.
18-24: LGTM! Networking setup is clear.The new networking section provides a clear starting point for infrastructure setup with a straightforward command to create the private VM network.
86-86: LGTM! Superset URL is clearly documented.The final public-facing URL for Superset is clearly specified with an appropriate domain placeholder.
|
|
||
| Now we deploy the remaining services. The deployment order is important as some | ||
| services depend on others being available. Each service has a single VM with the | ||
| exception of Superset that hosts multiple instances one once machine. |
There was a problem hiding this comment.
Fix typo in the sentence.
There's a typo: "one once machine" should be "on one machine".
Apply this diff:
-exception of Superset that hosts multiple instances one once machine.
+exception of Superset that hosts multiple instances on one machine.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exception of Superset that hosts multiple instances one once machine. | |
| exception of Superset that hosts multiple instances on one machine. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~56-~56: Possible missing preposition found.
Context: ... Superset that hosts multiple instances one once machine. First create the VMs: `...
(AI_HYDRA_LEO_MISSING_IN)
🤖 Prompt for AI Agents
In docs-devel/src/deployment/index.md around line 56, fix the typo in the
sentence fragment "exception of Superset that hosts multiple instances one once
machine" by replacing "one once machine" with "on one machine" so the sentence
reads correctly (e.g., "...exception of Superset that hosts multiple instances
on one machine").
|
Wrong base branch. Too many changes in this PR. |
1 participant
Summary
We no longer want to use data-accelerator.isis.cclrc.ac.uk. Remove the certificate that is about to expire.
Summary by CodeRabbit