docs(KEP): Test plan covering bring your own argo workflow feature #194
Conversation
WalkthroughTwo new documents have been added: a feature proposal and a comprehensive test plan for supporting "Bring Your Own Argo Workflows" in the Data Science Pipelines environment. These documents outline the requirements, architecture, migration considerations, and detailed validation strategy for enabling user-managed Argo Workflow Controllers instead of DSP-managed ones. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Cluster Admin
participant DSP as Data Science Pipelines (DSP)
participant Argo as User-provided Argo Workflow Controller
Admin->>DSP: Set global config to disable DSP-managed WorkflowController
DSP->>Argo: Integrate with user-provided Argo installation
Admin->>DSP: Deploy/upgrade DSP
DSP->>Argo: Submit workflow execution requests
Argo->>DSP: Return workflow status and results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Suggested labels
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
proposals/bring_your_own_argo_workflows/byoaw_context.md (4)
5-7: Fix typos in section heading and wording inside the rationale paragraphTwo quick wins:
- “Why doe we need this feature” → “Why do we need this feature”.
- “competition condition” → “contention condition” (or just “conflict”) is the more common phrasing.
-## Why doe we need this feature -... would create a competition condition between the user-provided and DSP-provisioned AWF instances ... +## Why do we need this feature +... would create a contention condition between the user-provided and DSP-provisioned AWF instances ...
76-78: Correct the “deploymentment” typo-DSPO already supports deploymentment of a Data Science Pipelines Application stack without a Workflow Controller +DSPO already supports deployment of a Data Science Pipelines Application stack without a Workflow Controller
99-105: Replace hard tabs with spaces to satisfy MD010The snippet below still uses tab characters in a YAML code block.
Markdown-lint (no-hard-tabs) will keep flagging this.- datasciencepipelines: - managementState: Managed - argoWorkflowsControllers: - managementState: Removed + datasciencepipelines: + managementState: Managed + argoWorkflowsControllers: + managementState: Removed
18-23: Punctuate “etc.” correctlyAmerican-English style expects a period after “etc”.
Search/replace a handful of occurrences (“etc” → “etc.”) to stay consistent.proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (2)
58-66: Add blank lines before and after the environment table to silence MD058Markdown-lint requires tables to be surrounded by blank lines.
-### Test Environments -| Environment | Argo Version | DSP Version | Purpose | +### Test Environments + +| Environment | Argo Version | DSP Version | Purpose | |-------------|--------------|-------------|---------| ... -| Env-4 | Various | Previous | Upgrade scenarios | +| Env-4 | Various | Previous | Upgrade scenarios | +
440-443: Tighten wording to avoid repetitionThe phrase “continues operating without issues” appears twice in the same bullet list.
Consider shortening to “remains functional” / “operates normally” for brevity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proposals/bring_your_own_argo_workflows/byoaw_context.md(1 hunks)proposals/bring_your_own_argo_workflows/byoaw_test_plan.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
proposals/bring_your_own_argo_workflows/byoaw_context.md
[grammar] ~5-~5: Ensure spelling is correct
Context: ...we validate this compatibility. ## Why doe we need this feature Potential customer...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ating a platform-level configuration. * As a Cluster Administrator I want to be ab...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~14-~14: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...that has Argo Workflows installation. * As a Cluster Administrator I want to manag...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~15-~15: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Workflow installation independently. * As a Cluster Administrator, I want to easi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: In American English, abbreviations like “etc.” require a period.
Context: ...lows, WorkflowTemplates, CronWorkflows, etc * Version of supported Argo Workflows, ...
(ETC_PERIOD)
[style] ~22-~22: This is not the usual sequence for adjectives that have no special emphasis.
Context: ...acked and tested for compatibility upon new minor releases * Example: ensure an ArgoWF ...
(EN_ADJ_ORDER)
[style] ~36-~36: In American English, abbreviations like “etc.” require a period.
Context: ...ipeline engine via k8s interfaces (CRs, etc). In turn, the Data Science Pipelines ...
(ETC_PERIOD)
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...pecially one that is deployed by a user outside of a Data Science Pipelines Application st...
(OUTSIDE_OF)
[style] ~38-~38: ‘with the intention of’ might be wordy. Consider a shorter alternative.
Context: ... they are working with this in mind and with the intention of introducing a written mandate that z-st...
(EN_WORDINESS_PREMIUM_WITH_THE_INTENTION_OF)
[grammar] ~57-~57: Ensure spelling is correct
Context: ...peline engine between user-provided and dspo-managed). That is - if a DSPA has a WC...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~57-~57: In American English, abbreviations like “etc.” require a period.
Context: ...eir own ArgoWF, how are runs, versions, etc persisted between the two Argos. As it ...
(ETC_PERIOD)
[grammar] ~76-~76: Ensure spelling is correct
Context: ...PO Implementation DSPO already supports deploymentment of a Data Science Pipelines Application...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~91-~91: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...that said, for RHOAI installations with a large number of DSPAs it would be unsustainable to requ...
(LARGE_NUMBER_OF)
[style] ~107-~107: In American English, abbreviations like “etc.” require a period.
Context: ...ing data, such as run details, metrics, etc) from an already-deployed DSPA. Curre...
(ETC_PERIOD)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md
[style] ~441-~441: Consider an alternative to avoid wordiness and to strengthen your wording.
Context: ...tallation
3. Verify RHOAI continues operating without issues
4. Perform independent upgrade of R...
(WORKS_WITHOUT_PROBLEMS)
[style] ~441-~441: Consider an alternative to avoid wordiness and to strengthen your wording.
Context: ...I
5. Verify external Argo continues operating without issues
6. Test independent scaling of each...
(WORKS_WITHOUT_PROBLEMS)
[uncategorized] ~508-~508: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ace mechanisms ## Risk Assessment ### High Risk - Data loss during migration scenarios ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~520-~520: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ed scenarios - Error handling gaps ### Low Risk - Minor UI/UX inconsistencies - Documen...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_context.md
99-99: Hard tabs
Column: 1
(MD010, no-hard-tabs)
100-100: Hard tabs
Column: 1
(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 3
(MD010, no-hard-tabs)
103-103: Hard tabs
Column: 5
(MD010, no-hard-tabs)
151-151: Bare URL used
(MD034, no-bare-urls)
155-155: Bare URL used
(MD034, no-bare-urls)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md
59-59: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pre-commit
- GitHub Check: check_ci_status
🔇 Additional comments (1)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (1)
506-525: Hyphenate compound adjectives“High risk”, “Medium risk”, “Low risk” are fine as nouns, but in sentences like “### High Risk – Data loss …” they’re effectively headings and acceptable.
However, if you embed them as modifiers later (“high-risk scenario”) add the hyphen.
Scanning shows none today, so only keep this in mind when reusing the list elsewhere.
|
A set of new images have been built to help with testing out this PR: |
|
An OCP cluster where you are logged in as cluster admin is required. The Data Science Pipelines team recommends testing this using the Data Science Pipelines Operator. Check here for more information on using the DSPO. To use and deploy a DSP stack with these images (assuming the DSPO is deployed), first save the following YAML to a file named apiVersion: datasciencepipelinesapplications.opendatahub.io/v1alpha1
kind: DataSciencePipelinesApplication
metadata:
name: pr-194
spec:
dspVersion: v2
apiServer:
image: "quay.io/opendatahub/ds-pipelines-api-server:pr-194"
argoDriverImage: "quay.io/opendatahub/ds-pipelines-driver:pr-194"
argoLauncherImage: "quay.io/opendatahub/ds-pipelines-launcher:pr-194"
persistenceAgent:
image: "quay.io/opendatahub/ds-pipelines-persistenceagent:pr-194"
scheduledWorkflow:
image: "quay.io/opendatahub/ds-pipelines-scheduledworkflow:pr-194"
mlmd:
deploy: true # Optional component
grpc:
image: "quay.io/opendatahub/mlmd-grpc-server:latest"
envoy:
image: "registry.redhat.io/openshift-service-mesh/proxyv2-rhel8:2.3.9-2"
mlpipelineUI:
deploy: true # Optional component
image: "quay.io/opendatahub/ds-pipelines-frontend:pr-194"
objectStorage:
minio:
deploy: true
image: 'quay.io/opendatahub/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance'Then run the following: cd $(mktemp -d)
git clone [email protected]:opendatahub-io/data-science-pipelines.git
cd data-science-pipelines/
git fetch origin pull/194/head
git checkout -b pullrequest 91ffe56f9434a3985de557a9978cc7095b6dfb1f
oc apply -f dspa.pr-194.yamlMore instructions here on how to deploy and test a Data Science Pipelines Application. |
|
/cc @gmfrasca |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (3)
59-65: Insert blank lines before & after the “Test Environments” table for Markdown compliance
markdownlint(MD058) flags this table because it is not surrounded by blank lines.
Add a blank line before| Environment |and another after the last table row to ensure correct rendering in most Markdown engines.-| Environment | Argo Version | DSP Version | Purpose | + +| Environment | Argo Version | DSP Version | Purpose | ... -| Env-4 | Various | Previous | Upgrade scenarios | +| Env-4 | Various | Previous | Upgrade scenarios | +
440-448: Section numbering skips from “7.2” to “7.2.1” – adjust hierarchy or renameThe sub-section heading
### 7.2.1 Z-Stream Version Compatibilitysits directly under### 7.2 Previous Version (N-1) Compatibility, but the parent uses H3 as well.
Either:
- Demote
7.2.1to H4 (####) to indicate it is a child of 7.2, or- Promote it to
7.3if it is meant to be a peer topic.Maintaining a clear heading hierarchy helps readers and tooling (TOC generators).
610-649: Consider splitting the “Test Deliverables” mega-list into subsections for readabilityEighteen bullet points in one block make scanning difficult. Group related deliverables (e.g., Performance, Compatibility, Security) under sub-headings or numbered lists to improve navigation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md
59-59: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
161-161: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
167-167: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
174-174: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
181-181: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
187-187: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
193-193: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
199-199: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
205-205: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
211-211: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
217-217: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
223-223: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
229-229: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
235-235: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
241-241: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
247-247: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pre-commit
- GitHub Check: check_ci_status
|
Change to PR detected. A new PR build was completed. |
nsingla
force-pushed
the
bring-your-own-argo-workflow-testplan
branch
from
8e0cac1 to
adaa867
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (2)
152-170: Table is still malformed ‒ inner “|” characters break the column countThe same MD056 issue reported earlier remains. Escape the pipes or, preferably, drop the table and switch to a heading + bullet-list pattern for each test case to avoid constant formatting breakage.
No new diff provided to avoid an enormous patch; previous suggestion still applies.
165-170:TC-PF-004identifier is duplicated – choose a unique ID
TC-PF-004appears both for “For loop constructs” (lines 165-170) and the multi-DSPA scenario (lines 273-276). Renumber the latter (e.g.TC-PF-021) and adjust any references.-273| | Test Case ID | TC-PF-004 | +273| | Test Case ID | TC-PF-021 |Also applies to: 273-276
🧹 Nitpick comments (1)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (1)
59-66: Surround the environment matrix with blank lines to satisfy MD058
markdownlintflags the table because it is not preceded and followed by a blank line.
Add a newline before line 60 and after line 66 to keep automated docs tooling quiet.59| ### Test Environments -60| | Environment | Argo Version | DSP Version | Purpose | +60| +| Environment | Argo Version | DSP Version | Purpose | ... -65| | Env-4 | Various | Previous | Upgrade scenarios | -66| +65| | Env-4 | Various | Previous | Upgrade scenarios | +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md
60-60: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
162-162: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
168-168: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
175-175: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
182-182: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
188-188: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
194-194: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
200-200: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
206-206: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
212-212: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
218-218: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
224-224: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
230-230: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
236-236: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
242-242: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
248-248: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pre-commit
- GitHub Check: check_ci_status
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (2)
153-177: Pipe characters inside table cells break the column count – please reformatThe internal
|symbols in the Test Steps and Expected Results cells cause markdownlint (MD056) to read these as 3-column tables, truncating content in many renderers. Escape the pipes or (preferably) replace the table with headings + bullet lists.-| **Test Steps** | 1. Execute pipeline - Pipelines with artifacts<br/>2. Verify each pipeline type executes correctly<br/>3. Validate artifacts, metadata, and custom configurations | +**Test Steps** +1. Execute pipeline – pipelines with artifacts +2. Verify each pipeline type executes correctly +3. Validate artifacts, metadata, and custom configurations
273-277: Duplicate Test Case IDTC-PF-004– assign a unique identifier
TC-PF-004is already used earlier (lines 165-170). Rename this Multi-DSPA test (e.g.TC-PF-021) and update every cross-reference to prevent automation/reporting clashes.-| Test Case ID | TC-PF-004 | +| Test Case ID | TC-PF-021 |
🧹 Nitpick comments (2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (2)
4-11: Verify ‘Table of Contents’ anchor slugsmarkdownlint (MD051) flags some fragment links (e.g.
#test-environment-requirements). Ensure each list entry matches the exact GitHub-generated slug (lower-case, spaces → hyphens, no punctuation) so in-page navigation works.
60-66: Add blank lines around the environment matrix tableA single blank line before and after the table satisfies MD058 and improves readability:
-### Test Environments -| Environment | Argo Version | DSP Version | Purpose | +### Test Environments + +| Environment | Argo Version | DSP Version | Purpose |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md
9-9: Link fragments should be valid
(MD051, link-fragments)
60-60: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
162-162: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
168-168: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
175-175: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
182-182: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
188-188: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
194-194: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
200-200: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
206-206: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
212-212: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
218-218: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
224-224: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
230-230: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
236-236: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
242-242: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
248-248: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pre-commit
- GitHub Check: check_ci_status
|
Change to PR detected. A new PR build was completed. |
1 similar comment
|
Change to PR detected. A new PR build was completed. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
proposals/bring_your_own_argo_workflows/byoaw_context.md (2)
5-7: Replace “customers” with “users” for OSS wording consistencyPrevious review already requested this terminology change but the text still reads “Potential customers”.
Please switch to “Potential users” to avoid implying a commercial relationship.
74-77: Fix typo “deploymentment”-DSPO already supports deploymentment of a Data Science Pipelines Application stack +DSPO already supports deployment of a Data Science Pipelines Application stackproposals/bring_your_own_argo_workflows/byoaw_test_plan.md (2)
164-169: Duplicate Test Case IDTC-PF-004
TC-PF-004already describes “For loop constructs” here, but is reused later for the multi-DSPA scenario.
Assign a unique ID to one of them and update any cross-references.-| Test Case ID | TC-PF-004 +| Test Case ID | TC-PF-021
152-176: Markdown tables break (MD056) due to unescaped “|”Inner pipes in the Test Steps and Expected Results cells are parsed as extra columns, so markdownlint sees 3 columns instead of 2.
Either:
- Escape each inner pipe (
\|), or- Replace the table with a heading + bullet list (easier to maintain for long multi-line content).
Example fix:
-| **Test Steps** | 1. Execute pipeline - Pipelines with artifacts<br/>2. Verify each pipeline type executes correctly<br/>3. Validate artifacts, metadata, and custom configurations | +#### Test Steps +1. Execute pipeline – pipelines with artifacts +2. Verify each pipeline type executes correctly +3. Validate artifacts, metadata, and custom configurations
🧹 Nitpick comments (2)
proposals/bring_your_own_argo_workflows/byoaw_context.md (1)
17-23: Add period after “etc.” (multiple occurrences)Markdownlint flags missing trailing periods after “etc”.
Add the period at the end of each instance for grammatical correctness.proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (1)
58-65: Add blank lines before and after the environment table (MD058)Add a newline above and below the table to satisfy markdownlint and improve readability.
+ | Environment | Argo Version | DSP Version | Purpose | ... | Env-4 | Various | Previous | Upgrade scenarios | +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proposals/bring_your_own_argo_workflows/byoaw_context.md(1 hunks)proposals/bring_your_own_argo_workflows/byoaw_test_plan.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
proposals/bring_your_own_argo_workflows/byoaw_context.md
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ating a platform-level configuration. * As a Cluster Administrator I want to be ab...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~14-~14: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...that has Argo Workflows installation. * As a Cluster Administrator I want to manag...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~15-~15: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Workflow installation independently. * As a Cluster Administrator, I want to easi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: In American English, abbreviations like “etc.” require a period.
Context: ...lows, WorkflowTemplates, CronWorkflows, etc * Version of supported Argo Workflows, ...
(ETC_PERIOD)
[style] ~22-~22: This is not the usual sequence for adjectives that have no special emphasis.
Context: ...acked and tested for compatibility upon new minor releases * Example: ensure an ArgoWF ...
(EN_ADJ_ORDER)
[style] ~36-~36: In American English, abbreviations like “etc.” require a period.
Context: ...ipeline engine via k8s interfaces (CRs, etc). In turn, the Data Science Pipelines ...
(ETC_PERIOD)
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...pecially one that is deployed by a user outside of a Data Science Pipelines Application st...
(OUTSIDE_OF)
[style] ~38-~38: ‘with the intention of’ might be wordy. Consider a shorter alternative.
Context: ... they are working with this in mind and with the intention of introducing a written mandate that z-st...
(EN_WORDINESS_PREMIUM_WITH_THE_INTENTION_OF)
[style] ~57-~57: In American English, abbreviations like “etc.” require a period.
Context: ...eir own ArgoWF, how are runs, versions, etc persisted between the two Argos. As it ...
(ETC_PERIOD)
[grammar] ~76-~76: Ensure spelling is correct
Context: ...PO Implementation DSPO already supports deploymentment of a Data Science Pipelines Application...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~91-~91: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...that said, for RHOAI installations with a large number of DSPAs it would be unsustainable to requ...
(LARGE_NUMBER_OF)
[style] ~107-~107: In American English, abbreviations like “etc.” require a period.
Context: ...ing data, such as run details, metrics, etc) from an already-deployed DSPA. Curre...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_context.md
99-99: Hard tabs
Column: 1
(MD010, no-hard-tabs)
100-100: Hard tabs
Column: 1
(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 3
(MD010, no-hard-tabs)
103-103: Hard tabs
Column: 5
(MD010, no-hard-tabs)
151-151: Bare URL used
(MD034, no-bare-urls)
155-155: Bare URL used
(MD034, no-bare-urls)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md
59-59: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
161-161: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
167-167: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
174-174: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
181-181: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
187-187: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
193-193: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
199-199: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
205-205: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
211-211: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
217-217: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
223-223: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
229-229: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
235-235: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
241-241: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
247-247: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (2)
270-281: Duplicate Test Case IDTC-PF-004– clash between sections
TC-PF-004is already used for the “For loop constructs” test.
The Multi-DSPA test re-uses the same ID; assign a unique identifier (e.g.TC-PF-023) to avoid tooling collisions.-| Test Case ID | TC-PF-004 +| Test Case ID | TC-PF-023
160-163: Steps / expected results mention artifacts for a without-artifacts testFor
TC-PF-003the pipeline explicitly runs without artifacts, yet the steps and expected results still require artifact validation.-**Test Steps** | 1. Execute Pipeline - Pipelines without artifacts<br/>2. Verify each pipeline type executes correctly<br/>3. Validate artifacts, metadata, and custom configurations | -**Expected Results** | - Pipeline runs successfully<br/>- Custom components and packages work correctly<br/>- Retry and iteration logic functions properly<br/>- Certificate handling operates securely<br/>- Metadata preserved correctly | +**Test Steps** | 1. Execute pipeline that produces **no** artifacts<br/>2. Verify execution completes without artifact storage<br/>3. Validate metadata and custom configurations | +**Expected Results** | - Pipeline runs successfully<br/>- No artifact objects created<br/>- Metadata recorded correctly<br/>- Retry / iteration logic functions properly |
🧹 Nitpick comments (3)
proposals/bring_your_own_argo_workflows/byoaw_context.md (2)
6-6: Use “users” instead of “customers” for OSS neutralityThe paragraph still says “Potential customers … prevents the customer from adopting DSP”.
Earlier feedback replaced “customers” with “users” to keep language vendor-neutral – please update here as well.-Potential customers, who have their own Argo Workflows installation already running on their clusters, … +Potential users, who have their own Argo Workflows installation already running on their clusters, … … -… therefore this prevents the customer from adopting DSP. +… therefore this prevents the user from adopting DSP.
75-77: Typo: duplicated word “deploymentment”Line 76 reads “deploymentment”.
-DSPO already supports deploymentment of a Data Science Pipelines Application stack +DSPO already supports deployment of a Data Science Pipelines Application stackproposals/bring_your_own_argo_workflows/byoaw_test_plan.md (1)
72-83: Markdown tables with inner pipes keep triggering MD056Tables such as
TC-CC-002declare two columns (|---|---|) but many cells contain extra un-escaped|, causing markdownlint failures and breaking GitHub rendering on narrower screens.Consider:
- Escape inner pipes with
\|, or- Convert each test case to a sub-heading plus bullet lists (much friendlier for long text).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proposals/bring_your_own_argo_workflows/byoaw_context.md(1 hunks)proposals/bring_your_own_argo_workflows/byoaw_test_plan.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
proposals/bring_your_own_argo_workflows/byoaw_context.md
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ating a platform-level configuration. * As a Cluster Administrator I want to be ab...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~14-~14: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...that has Argo Workflows installation. * As a Cluster Administrator I want to manag...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~15-~15: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Workflow installation independently. * As a Cluster Administrator, I want to easi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: In American English, abbreviations like “etc.” require a period.
Context: ...lows, WorkflowTemplates, CronWorkflows, etc * Version of supported Argo Workflows, ...
(ETC_PERIOD)
[style] ~22-~22: This is not the usual sequence for adjectives that have no special emphasis.
Context: ...acked and tested for compatibility upon new minor releases * Example: ensure an ArgoWF ...
(EN_ADJ_ORDER)
[style] ~36-~36: In American English, abbreviations like “etc.” require a period.
Context: ...ipeline engine via k8s interfaces (CRs, etc). In turn, the Data Science Pipelines ...
(ETC_PERIOD)
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...pecially one that is deployed by a user outside of a Data Science Pipelines Application st...
(OUTSIDE_OF)
[style] ~38-~38: ‘with the intention of’ might be wordy. Consider a shorter alternative.
Context: ... they are working with this in mind and with the intention of introducing a written mandate that z-st...
(EN_WORDINESS_PREMIUM_WITH_THE_INTENTION_OF)
[style] ~57-~57: In American English, abbreviations like “etc.” require a period.
Context: ...eir own ArgoWF, how are runs, versions, etc persisted between the two Argos. As it ...
(ETC_PERIOD)
[grammar] ~76-~76: Ensure spelling is correct
Context: ...PO Implementation DSPO already supports deploymentment of a Data Science Pipelines Application...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~91-~91: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...that said, for RHOAI installations with a large number of DSPAs it would be unsustainable to requ...
(LARGE_NUMBER_OF)
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md
59-59: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
161-161: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
167-167: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
174-174: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
181-181: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
187-187: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
193-193: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
199-199: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
205-205: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
211-211: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
217-217: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
223-223: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
229-229: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
235-235: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
241-241: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
247-247: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
274-274: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
|
Change to PR detected. A new PR build was completed. |
1 similar comment
|
Change to PR detected. A new PR build was completed. |
| | Test Case ID | TC-CC-006 | | ||
| |-----------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | **Test Case Summary** | Verify platform-level Argo CRDs and RBAC remain intact with external Argo | | ||
| | **Test Steps** | 1. Install DSPO which creates platform-level Argo CRDs and RBAC<br/>2. Install external Argo with different CRD versions<br/>3. Toggle global WorkflowController disable<br/>4. Verify platform CRDs are not removed<br/>5. Test that user modifications to CRDs are preserved<br/>6. Verify RBAC conflicts are handled appropriately | |
There was a problem hiding this comment.
This is more a document-encompassing comment, but should this test plan be in the DSPO repo instead, and/or perhaps we should split this test plan up into 2: one for DSPO testing and one for DSP w/ External Argo (No DSPO used) tests?
There was a problem hiding this comment.
for context, there are no references to DSPO (except for a couple instances in an internal procedure doc) in this entire repo), so I believe the intent is the DSP repo is 'unaware' of the existence of DSPO
There was a problem hiding this comment.
🤔 My thinking with this is that, DSP comes as a package with DSPO in a RHAOI cluster, so it doesn;t matter where this exist, but I see your point. I can move some of these tests to DSPO repo
There was a problem hiding this comment.
On second thoughts, I think its better to keep the whole test plan in a single place, otherwise we may miss to execute/implement certain portion of it
There was a problem hiding this comment.
if that is the case, then i think we should probably migrate this to the DSPO repo then, as the base DSP repo shouldn't have any real backreferences to the DSPOperator and/or platform operators (once this is ready to merge)
There was a problem hiding this comment.
Hmmmm..... good point
I can do that
There was a problem hiding this comment.
Actually, on second thoughts, as I was about to move this over, I am thinking that since the original proposal also exists here in this repo, we should probably leave the test plan here along with the proposal as well
There was a problem hiding this comment.
So FWIW I dont believe this repo has any BYOArgo materials in it either, including any proposals.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (2)
58-64: Hard-coded version strings risk rapid stalenessListing explicit Argo minor versions (3.7.x / 3.6.x / 3.4-3.5) may convey unintended support guarantees and will require frequent edits. Consider wording “Current (N)”, “Previous (N-1)”, “Legacy (>N-2)” or linking to a maintained compatibility matrix.
163-168: Duplicate Test Case IDTC-PF-004
TC-PF-004appears twice (“For loop constructs” and “Multi-DSPA Environment”).
Assign a unique ID to the latter (e.g.TC-PF-021) and adjust any cross-references.Also applies to: 276-280
🧹 Nitpick comments (2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (2)
1-1: Remove authoring metadata from final docThe “Assisted-by: Cursor” header is an editing artefact that adds no value to the published test plan and breaks the visual flow.
Please drop it.-**Assisted-by**: Cursor
4-11: Fix broken numbering in the Table of ContentsThe list jumps from item 4 to item 6, skipping 5. Markdown auto-numbering would avoid this slip and future drift.
-4. [Test Categories](#test-categories) -6. [Success Criteria](#success-criteria) +4. [Test Categories](#test-categories) +5. [Success Criteria](#success-criteria)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md
59-59: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
160-160: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
166-166: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
173-173: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
180-180: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
186-186: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
192-192: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
198-198: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
204-204: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
210-210: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
216-216: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
222-222: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
228-228: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
234-234: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
240-240: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
246-246: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
273-273: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pre-commit
- GitHub Check: check_ci_status
|
Change to PR detected. A new PR build was completed. |
nsingla
force-pushed
the
bring-your-own-argo-workflow-testplan
branch
from
916fd51 to
2ff5f84
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
proposals/bring_your_own_argo_workflows/byoaw_context.md (1)
94-103: YAML indentation still invalid –datasciencepipelinesmust nest undercomponents
datasciencepipelines:is aligned withcomponents:instead of being indented two additional spaces. Tools copying this manifest will fail to parse.spec: components: - datasciencepipelines: - managementState: Managed - argoWorkflowsControllers: - managementState: Removed + datasciencepipelines: + managementState: Managed + argoWorkflowsControllers: + managementState: Removedproposals/bring_your_own_argo_workflows/byoaw_test_plan.md (4)
158-163: Same formatting bug repeats (TC-PF-003)
Trailing pipe and missing</ol>.
256-269: Unclosed/incorrectly-closed<ol>tags in Pod-spec override section
Each of these tables finishes the Test Steps row with<ol>instead of</ol>and keeps the phantom third column alive.Refactor one and repeat for TC-PF-019/020/021:
115-122: Malformed heading + unclosed ordered list
### <ol><li>6 Sub-Component Removal Testingrenders literally and breaks the document outline.
The ordered list inside Test Steps is opened but never closed, so everything after this row is treated as part of the same list....
152-157: Extra trailing pipe ⇒ column-count mismatch (MD056)
The final|creates a phantom third column. Remove it and close the<ol>list.
🧹 Nitpick comments (9)
proposals/bring_your_own_argo_workflows/byoaw_context.md (6)
1-3: Remove redundant wording & tighten sentence“...and instead use user-provided Argo Workflows instead” repeats instead and reads awkwardly.
-...and instead use user-provided Argo Workflows instead. +...and use user-provided Argo Workflows.
5-7: Replace non-standard phrase “competition condition”The common term is “contention” or “conflict”, not “competition condition”.
-This would create a competition condition between the user-provided... +This would create a conflict between the user-provided...
11-16: Consider consolidating repetitive “As a Cluster Administrator…” bulletsFive successive bullets start with the identical phrase, which hurts scan-ability. Group related requirements or drop the leading clause after the first occurrence to reduce noise.
18-23: Finish abbreviation & minor punctuationMissing period after “etc” and a stray comma after “releases”.
-* Includes Workflows, WorkflowTemplates, CronWorkflows, etc -* Version of supported Argo Workflows, and latest version of n-1 previous minor release, would need to be tracked and tested for compatibility upon new minor releases +* Includes Workflows, WorkflowTemplates, CronWorkflows, etc. +* Versions of supported Argo Workflows and the latest version of the n-1 previous minor release must be tracked and tested for compatibility upon new minor releases.
40-41: Break extremely long sentence for readabilityLines 40-41 run >40 words and embed two parentheticals. Splitting after “…over a year.” improves comprehension.
78-89: Specify fenced-code language for YAML snippetAdding the language identifier enables syntax highlighting and prevents some Markdown linters from flagging the block.
-```--- +```yaml +---proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (3)
80-82: Mixed HTML & markdown list syntax — readability + maintenance
The Test Steps rows rely on raw HTML ordered lists. For long tables this becomes painful to maintain (closing tags forgotten multiple times). Consider switching to plain markdown inside cells or, even better, abandoning tables in favour of heading + bullet list as already suggested in past reviews.
This will eliminate almost every MD056/MD058 lint error in the file.
4-11: TOC numbering skips “5.”
The index jumps from 4 to 6. Either renumber manually or use an auto-generated TOC tool to avoid drift.
127-130: Pre-existing Argo detection steps: missing code-block fences
The YAML snippet referenced in Step 3 (global disable) is shown earlier as inline code. For clarity and copy-paste friendliness, wrap multi-line YAML in a fenced block or keep it one-liner but with proper indentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proposals/bring_your_own_argo_workflows/byoaw_context.md(1 hunks)proposals/bring_your_own_argo_workflows/byoaw_test_plan.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
proposals/bring_your_own_argo_workflows/byoaw_context.md
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ating a platform-level configuration. * As a Cluster Administrator I want to be ab...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~14-~14: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...that has Argo Workflows installation. * As a Cluster Administrator I want to manag...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~15-~15: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Workflow installation independently. * As a Cluster Administrator, I want to easi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: In American English, abbreviations like “etc.” require a period.
Context: ...lows, WorkflowTemplates, CronWorkflows, etc * Version of supported Argo Workflows, ...
(ETC_PERIOD)
[style] ~22-~22: This is not the usual sequence for adjectives that have no special emphasis.
Context: ...acked and tested for compatibility upon new minor releases * Example: ensure an ArgoWF ...
(EN_ADJ_ORDER)
[style] ~36-~36: In American English, abbreviations like “etc.” require a period.
Context: ...ipeline engine via k8s interfaces (CRs, etc). In turn, the Data Science Pipelines ...
(ETC_PERIOD)
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...pecially one that is deployed by a user outside of a Data Science Pipelines Application st...
(OUTSIDE_OF)
[style] ~38-~38: ‘with the intention of’ might be wordy. Consider a shorter alternative.
Context: ... they are working with this in mind and with the intention of introducing a written mandate that z-st...
(EN_WORDINESS_PREMIUM_WITH_THE_INTENTION_OF)
[grammar] ~57-~57: Ensure spelling is correct
Context: ...peline engine between user-provided and dspo-managed). That is - if a DSPA has a WC...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~57-~57: In American English, abbreviations like “etc.” require a period.
Context: ...eir own ArgoWF, how are runs, versions, etc persisted between the two Argo Workflow...
(ETC_PERIOD)
[style] ~91-~91: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...h that said, for ODH installations with a large number of DSPAs it would be unsustainable to requ...
(LARGE_NUMBER_OF)
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md
59-59: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
161-161: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
167-167: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
174-174: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
nsingla
force-pushed
the
bring-your-own-argo-workflow-testplan
branch
from
2ff5f84 to
29915c4
Compare
|
Change to PR detected. A new PR build was completed. |
1 similar comment
|
Change to PR detected. A new PR build was completed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
proposals/bring_your_own_argo_workflows/byoaw_context.md (1)
94-103: YAML snippet still mis-indented undercomponents
The previous suggestion has not been fully applied:datasciencepipelines:must be nested undercomponents:.spec: components: - datasciencepipelines: - managementState: Managed - argoWorkflowsControllers: - managementState: Removed + datasciencepipelines: + managementState: Managed + argoWorkflowsControllers: + managementState: Removedproposals/bring_your_own_argo_workflows/byoaw_test_plan.md (2)
161-168: Trailing “|” still breaks table column count (MD056)These rows still have three columns instead of two because of the extra pipe at the end. They also contain spaced list items; stripping the pipe is enough to make markdownlint happy.
-| **Test Steps** | <ol><li> Configure DSPA with external Argo </li><li> Execute Pipeline - Pipelines without artifacts</li><li> Verify each pipeline type executes correctly</li></ol> | | +| **Test Steps** | <ol><li>Configure DSPA with external Argo</li><li>Execute pipeline – pipelines without artifacts</li><li>Verify each pipeline type executes correctly</li></ol> | ... -| **Test Steps** | <ol><li> Configure DSPA with external Argo </li><li> Execute Pipeline - For loop constructs</li><li> Verify each pipeline type executes correctly</li></ol> | | +| **Test Steps** | <ol><li>Configure DSPA with external Argo</li><li>Execute pipeline – for-loop constructs</li><li>Verify each pipeline type executes correctly</li></ol> | ... -| **Test Steps** | <ol><li> Configure DSPA with external Argo </li><li> Execute Pipeline - Parallel for execution</li><li> Verify each pipeline type executes correctly</li></ol> | | +| **Test Steps** | <ol><li>Configure DSPA with external Argo</li><li>Execute pipeline – parallel-for execution</li><li>Verify each pipeline type executes correctly</li></ol> |Also applies to: 174-175
115-116: Remove stray<ol><li>markup from headingThe heading currently contains raw HTML and an incorrect sequence number, breaking the document outline and some renderers.
-### <ol><li>6 Sub-Component Removal Testing +### 1.6 Sub-Component Removal Testing
🧹 Nitpick comments (5)
proposals/bring_your_own_argo_workflows/byoaw_context.md (3)
2-3: Remove duplicated word “instead”The sentence already states “to disable … and instead use user-provided Argo Workflows”.
The trailing “instead” is redundant.-This feature will implement a global configuration option to disable WorkflowControllers from being deployed alongside DataSciencePipelineApplications, and instead use user-provided Argo Workflows instead. +This feature will implement a global configuration option to disable WorkflowControllers from being deployed alongside DataSciencePipelineApplications, and instead use user-provided Argo Workflows.
6-6: Replace “competition condition” with “contention”“Contention” (or “conflict”) is the conventional term for two controllers reacting to the same events.
-This would create a competition condition between the user-provided and DSP-provisioned AWF instances, +This would create contention between the user-provided and DSP-provisioned AWF instances,
78-90: Use proper fenced-code syntax highlighting
```---is not a recognised info-string and prevents syntax highlighting.
Specify the language (yamlis appropriate) and drop the stray dashes.-```--- +```yaml # DataSciencePipelinesApplication with WC disabled apiVersion: datasciencepipelinesapplications.opendatahub.io/v1 …</blockquote></details> <details> <summary>proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (2)</summary><blockquote> `58-64`: **Add blank line before table to clear MD058 lint error** `markdownlint` still flags the table because it is glued to the heading. Rendering engines also merge heading text and first row. ```diff -### Test Environments -| Environment | Argo Version | DSP Version | Purpose | +### Test Environments + +| Environment | Argo Version | DSP Version | Purpose |
74-75: Normalize list markup inside table cellMixed
<li>tags with “1.” bullets confuse both renderers and linters. Use pure HTML or pure Markdown; don’t mix.-| **Test Steps** | <ol><li>Install ODH with default configuration (WorkflowControllers enabled)<br/>1. Create DSPA and verify WorkflowController deployment<br/>1. Update DataScienceCluster to disable WorkflowControllers:<br/>`spec.components.datasciencepipelines.argoWorkflowsControllers.managementState: Removed`<br/>1. Verify existing WorkflowControllers are removed<br/>1. Create new DSPA and verify no WorkflowController is deployed</li></ol> | +| **Test Steps** | <ol><li>Install ODH with default configuration (WorkflowControllers enabled)</li><li>Create DSPA and verify WorkflowController deployment</li><li>Update DataScienceCluster to disable WorkflowControllers:<br/>`spec.components.datasciencepipelines.argoWorkflowsControllers.managementState: Removed`</li><li>Verify existing WorkflowControllers are removed</li><li>Create new DSPA and verify no WorkflowController is deployed</li></ol> |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proposals/bring_your_own_argo_workflows/byoaw_context.md(1 hunks)proposals/bring_your_own_argo_workflows/byoaw_test_plan.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md
59-59: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
161-161: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
167-167: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
174-174: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
🪛 LanguageTool
proposals/bring_your_own_argo_workflows/byoaw_context.md
[uncategorized] ~9-~9: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...his block. ## Feature Requirements ### High level requirements * As a Cluster Administrat...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ating a platform-level configuration. * As a Cluster Administrator I want to be ab...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~14-~14: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...that has Argo Workflows installation. * As a Cluster Administrator I want to manag...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~15-~15: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Workflow installation independently. * As a Cluster Administrator, I want to easi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: In American English, abbreviations like “etc.” require a period.
Context: ...lows, WorkflowTemplates, CronWorkflows, etc * Version of supported Argo Workflows, ...
(ETC_PERIOD)
[style] ~22-~22: This is not the usual sequence for adjectives that have no special emphasis.
Context: ...acked and tested for compatibility upon new minor releases * Example: ensure an ArgoWF ...
(EN_ADJ_ORDER)
[style] ~36-~36: In American English, abbreviations like “etc.” require a period.
Context: ...ipeline engine via k8s interfaces (CRs, etc). In turn, the Data Science Pipelines ...
(ETC_PERIOD)
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...pecially one that is deployed by a user outside of a Data Science Pipelines Application st...
(OUTSIDE_OF)
[style] ~38-~38: ‘with the intention of’ might be wordy. Consider a shorter alternative.
Context: ... they are working with this in mind and with the intention of introducing a written mandate that z-st...
(EN_WORDINESS_PREMIUM_WITH_THE_INTENTION_OF)
[style] ~57-~57: In American English, abbreviations like “etc.” require a period.
Context: ...eir own ArgoWF, how are runs, versions, etc persisted between the two Argo Workflow...
(ETC_PERIOD)
[style] ~91-~91: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...h that said, for ODH installations with a large number of DSPAs it would be unsustainable to requ...
(LARGE_NUMBER_OF)
|
Change to PR detected. A new PR build was completed. |
1 similar comment
|
Change to PR detected. A new PR build was completed. |
nsingla
force-pushed
the
bring-your-own-argo-workflow-testplan
branch
from
99534d4 to
4d90b72
Compare
|
Change to PR detected. A new PR build was completed. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
proposals/bring_your_own_argo_workflows/byoaw_context.md (1)
97-102: Fix YAML indentation – snippet is still structurally invalid
datasciencepipelines:must be nested undercomponents:. As-is, the example would be rejected by the API server and misleads readers.- components: - datasciencepipelines: + components: + datasciencepipelines: managementState: Managed argoWorkflowsControllers: managementState: Removedproposals/bring_your_own_argo_workflows/byoaw_test_plan.md (2)
57-63: Add mandatory blank lines around the table to satisfy MD058 and improve renderingThe heading and the table are glued together.
markdownlintflags this (MD058), and some renderers will swallow the heading paragraph.-### Test Environments -| Environment | Argo Version | DSP Version | Purpose | +### Test Environments + +| Environment | Argo Version | DSP Version | Purpose | ... -| Env-3 | 3.4.x - 3.5.x | Previous | Upgrade scenarios | +| Env-3 | 3.4.x - 3.5.x | Previous | Upgrade scenarios | +
120-126: Heading still contains raw HTML and wrong numberingThe
<ol><li>prefix is invalid inside a Markdown heading and the section number is off (should be 1.6).-### <ol><li>6 Sub-Component Removal Testing +### 1.6 Sub-Component Removal Testing
🧹 Nitpick comments (5)
proposals/bring_your_own_argo_workflows/byoaw_context.md (5)
78-89: Use proper code-block fences and YAML header
```---is not a valid fence and disables syntax highlighting.
Switch to language-tagged triple back-ticks and drop the literal---line.-```--- +```yaml apiVersion: datasciencepipelinesapplications.opendatahub.io/v1 … workflowController: deploy: false ... - ``` +```
29-30: Collapse duplicated bullets on compatibility-testing strategyLines 29 – 30 describe the same requirement twice; merge them into a single bullet to avoid confusion.
21-22: Add the missing period after “etc.”For consistency with American English and prior occurrences, append the period after
etcin these bullets/sentences.Also applies to: 36-36, 57-57
8-10: Hyphenate compound adjective in heading“High level requirements” → “High-level requirements” (compound adjective before a noun). This also aligns with the style used elsewhere.
5-5: Consider rephrasing heading for clarityUsing “## Motivation” or adding a question mark (“## Why do we need this feature?”) improves readability and matches common design-doc conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proposals/bring_your_own_argo_workflows/byoaw_context.md(1 hunks)proposals/bring_your_own_argo_workflows/byoaw_test_plan.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
proposals/bring_your_own_argo_workflows/byoaw_context.md
[uncategorized] ~9-~9: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...his block. ## Feature Requirements ### High level requirements * As a Cluster Administrat...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ating a platform-level configuration. * As a Cluster Administrator I want to be ab...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~14-~14: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...that has Argo Workflows installation. * As a Cluster Administrator I want to manag...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~15-~15: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Workflow installation independently. * As a Cluster Administrator, I want to easi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: In American English, abbreviations like “etc.” require a period.
Context: ...lows, WorkflowTemplates, CronWorkflows, etc * Version of supported Argo Workflows, ...
(ETC_PERIOD)
[style] ~22-~22: This is not the usual sequence for adjectives that have no special emphasis.
Context: ...acked and tested for compatibility upon new minor releases * Example: ensure an ArgoWF ...
(EN_ADJ_ORDER)
[style] ~36-~36: In American English, abbreviations like “etc.” require a period.
Context: ...ipeline engine via k8s interfaces (CRs, etc). In turn, the Data Science Pipelines ...
(ETC_PERIOD)
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...pecially one that is deployed by a user outside of a Data Science Pipelines Application st...
(OUTSIDE_OF)
[style] ~38-~38: ‘with the intention of’ might be wordy. Consider a shorter alternative.
Context: ... they are working with this in mind and with the intention of introducing a written mandate that z-st...
(EN_WORDINESS_PREMIUM_WITH_THE_INTENTION_OF)
[style] ~57-~57: In American English, abbreviations like “etc.” require a period.
Context: ...eir own ArgoWF, how are runs, versions, etc persisted between the two Argo Workflow...
(ETC_PERIOD)
[style] ~91-~91: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...h that said, for ODH installations with a large number of DSPAs it would be unsustainable to requ...
(LARGE_NUMBER_OF)
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md
58-58: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
166-166: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
172-172: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
179-179: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check_ci_status
| | Test Case ID | TC-CC-001 | | ||
| |-----------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | **Test Case Summary** | Verify global toggle to disable WorkflowControllers works correctly | | ||
| | **Test Steps** | <ol><li>Install ODH with default configuration (WorkflowControllers enabled)<br/>1. Create DSPA and verify WorkflowController deployment<br/>1. Update DataScienceCluster to disable WorkflowControllers:<br/>`spec.components.datasciencepipelines.argoWorkflowsControllers.managementState: Removed`<br/>1. Verify existing WorkflowControllers are removed<br/>1. Create new DSPA and verify no WorkflowController is deployed</li></ol> | | ||
| | **Expected Results** | - Global toggle successfully disables WorkflowController deployment<br/>- Existing WorkflowControllers are cleanly removed<br/>- New DSPAs respect global configuration<br/>- No data loss during WorkflowController removal | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Malformed ordered-list markup inside the “Test Steps” cell
Only one <li> tag is opened, yet the subsequent items are introduced with 1. and never wrapped in <li> tags. This breaks HTML validity and renders unpredictably.
-| **Test Steps** | <ol><li>Install ODH with default configuration (WorkflowControllers enabled)<br/>1. Create DSPA and verify WorkflowController deployment<br/>1. Update DataScienceCluster to disable WorkflowControllers:<br/>`spec.components.datasciencepipelines.argoWorkflowsControllers.managementState: Removed`<br/>1. Verify existing WorkflowControllers are removed<br/>1. Create new DSPA and verify no WorkflowController is deployed</li></ol> |
+| **Test Steps** | <ol> <li>Install ODH with default configuration (WorkflowControllers enabled)</li> <li>Create DSPA and verify WorkflowController deployment</li> <li>Update DataScienceCluster to disable WorkflowControllers:<br/>`spec.components.datasciencepipelines.argoWorkflowsControllers.managementState: Removed`</li> <li>Verify existing WorkflowControllers are removed</li> <li>Create new DSPA and verify no WorkflowController is deployed</li> </ol> |📝 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.
| | Test Case ID | TC-CC-001 | | |
| |-----------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | |
| | **Test Case Summary** | Verify global toggle to disable WorkflowControllers works correctly | | |
| | **Test Steps** | <ol><li>Install ODH with default configuration (WorkflowControllers enabled)<br/>1. Create DSPA and verify WorkflowController deployment<br/>1. Update DataScienceCluster to disable WorkflowControllers:<br/>`spec.components.datasciencepipelines.argoWorkflowsControllers.managementState: Removed`<br/>1. Verify existing WorkflowControllers are removed<br/>1. Create new DSPA and verify no WorkflowController is deployed</li></ol> | | |
| | **Expected Results** | - Global toggle successfully disables WorkflowController deployment<br/>- Existing WorkflowControllers are cleanly removed<br/>- New DSPAs respect global configuration<br/>- No data loss during WorkflowController removal | | |
| | Test Case ID | TC-CC-001 | | |
| |-----------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | |
| | **Test Case Summary** | Verify global toggle to disable WorkflowControllers works correctly | | |
| | **Test Steps** | <ol> <li>Install ODH with default configuration (WorkflowControllers enabled)</li> <li>Create DSPA and verify WorkflowController deployment</li> <li>Update DataScienceCluster to disable WorkflowControllers:<br/>`spec.components.datasciencepipelines.argoWorkflowsControllers.managementState: Removed`</li> <li>Verify existing WorkflowControllers are removed</li> <li>Create new DSPA and verify no WorkflowController is deployed</li> </ol> | | |
| | **Expected Results** | - Global toggle successfully disables WorkflowController deployment<br/>- Existing WorkflowControllers are cleanly removed<br/>- New DSPAs respect global configuration<br/>- No data loss during WorkflowController removal | |
🤖 Prompt for AI Agents
In proposals/bring_your_own_argo_workflows/byoaw_test_plan.md lines 70 to 74,
the "Test Steps" cell contains a malformed ordered list where only the first
step is wrapped in an <li> tag and subsequent steps use "1." without <li> tags.
Fix this by wrapping each test step inside its own <li> tag within the <ol> to
ensure valid HTML and proper rendering of the ordered list.
| | Test Case ID | TC-CC-003 | | ||
| |-----------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | **Test Case Summary** | Verify BYOAW compatibility with Kubernetes Native Mode - Create Pipeline Via CR | | ||
| | **Test Steps** | <ol><li>Configure cluster for Kubernetes Native Mode</li><li>Install external Argo Workflows</li><li>Disable DSP WorkflowControllers globally</li><li>Create DSPA</li><li>Create Pipeline via CR and create a pipeline run</li></ol> | | ||
| | **Expected Results** | - Kubernetes Native Mode works with external Argo<br/>- Pipeline execution uses Kubernetes-native constructs<br/>- No conflicts between modes | | ||
|
|
||
| | Test Case ID | TC-CC-003 | | ||
| |-----------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | **Test Case Summary** | Verify BYOAW compatibility with Kubernetes Native Mode - Create Pipeline via API | | ||
| | **Test Steps** | <ol><li>Configure cluster for Kubernetes Native Mode</li><li>Install external Argo Workflows</li><li>Disable DSP WorkflowControllers globally</li><li>Create DSPA</li><li>Create Pipeline via API/UI and create a pipeline run</li></ol> | | ||
| | **Expected Results** | - Kubernetes Native Mode works with external Argo<br/>- Pipeline executes successfully<br/> | | ||
|
|
There was a problem hiding this comment.
Duplicate Test-Case ID TC-CC-003 – must be unique
Both Kubernetes-native tests use the same identifier. Duplicate IDs break traceability in TestRail / Zephyr and confuse automation.
-| Test Case ID | TC-CC-003
+| Test Case ID | TC-CC-003a
...
-| Test Case ID | TC-CC-003
+| Test Case ID | TC-CC-003b📝 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.
| | Test Case ID | TC-CC-003 | | |
| |-----------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | |
| | **Test Case Summary** | Verify BYOAW compatibility with Kubernetes Native Mode - Create Pipeline Via CR | | |
| | **Test Steps** | <ol><li>Configure cluster for Kubernetes Native Mode</li><li>Install external Argo Workflows</li><li>Disable DSP WorkflowControllers globally</li><li>Create DSPA</li><li>Create Pipeline via CR and create a pipeline run</li></ol> | | |
| | **Expected Results** | - Kubernetes Native Mode works with external Argo<br/>- Pipeline execution uses Kubernetes-native constructs<br/>- No conflicts between modes | | |
| | Test Case ID | TC-CC-003 | | |
| |-----------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | |
| | **Test Case Summary** | Verify BYOAW compatibility with Kubernetes Native Mode - Create Pipeline via API | | |
| | **Test Steps** | <ol><li>Configure cluster for Kubernetes Native Mode</li><li>Install external Argo Workflows</li><li>Disable DSP WorkflowControllers globally</li><li>Create DSPA</li><li>Create Pipeline via API/UI and create a pipeline run</li></ol> | | |
| | **Expected Results** | - Kubernetes Native Mode works with external Argo<br/>- Pipeline executes successfully<br/> | | |
| | Test Case ID | TC-CC-003a | | |
| |-----------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | |
| | **Test Case Summary** | Verify BYOAW compatibility with Kubernetes Native Mode - Create Pipeline Via CR | | |
| | **Test Steps** | <ol><li>Configure cluster for Kubernetes Native Mode</li><li>Install external Argo Workflows</li><li>Disable DSP WorkflowControllers globally</li><li>Create DSPA</li><li>Create Pipeline via CR and create a pipeline run</li></ol> | | |
| | **Expected Results** | - Kubernetes Native Mode works with external Argo<br/>- Pipeline execution uses Kubernetes-native constructs<br/>- No conflicts between modes | | |
| | Test Case ID | TC-CC-003b | | |
| |-----------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | |
| | **Test Case Summary** | Verify BYOAW compatibility with Kubernetes Native Mode - Create Pipeline via API | | |
| | **Test Steps** | <ol><li>Configure cluster for Kubernetes Native Mode</li><li>Install external Argo Workflows</li><li>Disable DSP WorkflowControllers globally</li><li>Create DSPA</li><li>Create Pipeline via API/UI and create a pipeline run</li></ol> | | |
| | **Expected Results** | - Kubernetes Native Mode works with external Argo<br/>- Pipeline executes successfully<br/> | |
🤖 Prompt for AI Agents
In proposals/bring_your_own_argo_workflows/byoaw_test_plan.md around lines 84 to
95, the test cases for Kubernetes Native Mode have duplicate Test Case IDs
"TC-CC-003". Update one of the test case IDs to a unique identifier to ensure
traceability and avoid confusion in test management tools.
| |-----------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | **Test Case Summary** | Verify run of "Pipelines without artifacts" pipeline | | ||
| | **Test Steps** | <ol><li> Configure DSPA with external Argo </li><li> Execute Pipeline - Pipelines without artifacts</li><li> Verify each pipeline type executes correctly</li></ol> | | | ||
| | **Expected Results** | - Pipeline runs successfully<br/>- No artifacts are produced to S3 | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extra trailing pipe causes MD056 (column-count) errors
The closing | after the cell content creates a phantom third column.
-| **Test Steps** | <ol><li> Configure DSPA with external Argo </li><li> Execute Pipeline - Pipelines without artifacts</li><li> Verify each pipeline type executes correctly</li></ol> | |
+| **Test Steps** | <ol><li>Configure DSPA with external Argo</li><li>Execute pipeline – pipelines without artifacts</li><li>Verify each pipeline type executes correctly</li></ol> |Repeat the same fix for the TC-PF-004 and TC-PF-005 rows (Lines 171-173 and 178-180).
📝 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.
| |-----------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------| | |
| | **Test Case Summary** | Verify run of "Pipelines without artifacts" pipeline | | |
| | **Test Steps** | <ol><li> Configure DSPA with external Argo </li><li> Execute Pipeline - Pipelines without artifacts</li><li> Verify each pipeline type executes correctly</li></ol> | | | |
| | **Expected Results** | - Pipeline runs successfully<br/>- No artifacts are produced to S3 | | |
| |-----------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------| | |
| | **Test Case Summary** | Verify run of "Pipelines without artifacts" pipeline | | |
| | **Test Steps** | <ol><li>Configure DSPA with external Argo</li><li>Execute pipeline – pipelines without artifacts</li><li>Verify each pipeline type executes correctly</li></ol> | | |
| | **Expected Results** | - Pipeline runs successfully<br/>- No artifacts are produced to S3 | |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
166-166: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
In proposals/bring_your_own_argo_workflows/byoaw_test_plan.md around lines 164
to 167, remove the extra trailing pipe character at the end of each table row to
fix the MD056 column-count errors. Apply the same fix to the rows for TC-PF-004
and TC-PF-005 at lines 171-173 and 178-180 by deleting the trailing pipe to
ensure consistent column counts in the markdown table.
|
/lgtm |
nsingla
force-pushed
the
bring-your-own-argo-workflow-testplan
branch
from
4d90b72 to
301e143
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
proposals/bring_your_own_argo_workflows/byoaw_context.md (1)
97-103: Fix invalid YAML indentation in the DSC example
datasciencepipelines:must be nested undercomponents:– the current alignment produces an invalid object tree and will mislead anyone copy-pasting the snippet.spec: components: - datasciencepipelines: - managementState: Managed - argoWorkflowsControllers: - managementState: Removed + datasciencepipelines: + managementState: Managed + argoWorkflowsControllers: + managementState: Removedproposals/bring_your_own_argo_workflows/byoaw_test_plan.md (4)
85-96: Duplicate Test-Case IDTC-CC-003persists
The same ID is used for both Kubernetes-native tests, breaking traceability in test management tooling.-| Test Case ID | TC-CC-003 +| Test Case ID | TC-CC-003a ... -| Test Case ID | TC-CC-003 +| Test Case ID | TC-CC-003bUpdate any internal links or references accordingly.
121-128: Malformed heading mixes raw HTML and wrong numbering
### <ol><li>6 Sub-Component Removal Testingis invalid Markdown and renders unpredictably.-### <ol><li>6 Sub-Component Removal Testing +### 1.6 Sub-Component Removal TestingRemove the stray HTML tags and keep consistent section numbering.
165-175: Extra trailing pipe causes MD056 column-count errors
Rows forTC-PF-003andTC-PF-004still end with an extra|, producing a phantom third column.-| **Test Steps** | <ol><li> Configure DSPA … executes correctly</li></ol> | +| **Test Steps** | <ol><li>Configure DSPA … executes correctly</li></ol> |Remove the final pipe character for all affected rows to restore a two-column table.
74-75: Ordered-list markup inside the “Test Steps” cell is still malformed
Only the first list item is wrapped in<li>, subsequent items use1.without tags, resulting in invalid HTML.-<ol><li>Install ODH … enabled)<br/>1. Create DSPA … </li></ol> +<ol> + <li>Install ODH … enabled)</li> + <li>Create DSPA …</li> + <li>Update DataScienceCluster …</li> + <li>Verify existing WorkflowControllers are removed</li> + <li>Create new DSPA …</li> +</ol>Wrap each step in its own
<li>(or switch to plain Markdown lists) for proper rendering.
🧹 Nitpick comments (4)
proposals/bring_your_own_argo_workflows/byoaw_context.md (3)
78-89: Use a proper code-fence header for YAMLThe fence string
---is non-standard. Using a language hint improves rendering and editor linting.-```--- +```yaml --- # … body unchanged …--- `29-30`: **Consolidate the duplicated testing-strategy bullets** The two bullets convey the same idea (N and N-1 coverage). Merging them tightens the list and avoids reader confusion. ```diff -* Update, improve and document a testing strategy for coverage of supported versions of Argo workflows for a given ODH version -* Update, improve and document a testing strategy for coverage of latest version of previous minor release of Argo Workflows for a given ODH version +* Update, improve, and document a testing strategy that validates: + * The Argo Workflows versions officially supported by the given ODH release + * The latest patch of the previous minor Argo Workflows release (N-1)
21-22: Minor copy-editing – add the period after “etc.”Several occurrences of “etc” are missing the trailing period, which is required in American English and caught by markdownlint/LanguageTool.
(Instances on Lines 21–22, 36, 57, 138.)
No functional impact, but worth cleaning up while you’re touching the doc.
Also applies to: 36-36, 57-57, 138-138
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md (1)
59-65: Missing blank line after heading triggers MD058
Add an empty line between the “### Test Environments” heading and the table to satisfy markdownlint.-### Test Environments -| Environment | Argo Version | DSP Version | Purpose | +### Test Environments + +| Environment | Argo Version | DSP Version | Purpose |A trailing blank line after the table is also recommended for readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proposals/bring_your_own_argo_workflows/byoaw_context.md(1 hunks)proposals/bring_your_own_argo_workflows/byoaw_test_plan.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
proposals/bring_your_own_argo_workflows/byoaw_context.md
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ating a platform-level configuration. * As a Cluster Administrator I want to be ab...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~14-~14: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...that has Argo Workflows installation. * As a Cluster Administrator I want to manag...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~15-~15: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Workflow installation independently. * As a Cluster Administrator, I want to easi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: In American English, abbreviations like “etc.” require a period.
Context: ...lows, WorkflowTemplates, CronWorkflows, etc * Version of supported Argo Workflows, ...
(ETC_PERIOD)
[style] ~22-~22: This is not the usual sequence for adjectives that have no special emphasis.
Context: ...acked and tested for compatibility upon new minor releases * Example: ensure an ArgoWF ...
(EN_ADJ_ORDER)
[style] ~36-~36: In American English, abbreviations like “etc.” require a period.
Context: ...ipeline engine via k8s interfaces (CRs, etc). In turn, the Data Science Pipelines ...
(ETC_PERIOD)
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...pecially one that is deployed by a user outside of a Data Science Pipelines Application st...
(OUTSIDE_OF)
[style] ~38-~38: ‘with the intention of’ might be wordy. Consider a shorter alternative.
Context: ... they are working with this in mind and with the intention of introducing a written mandate that z-st...
(EN_WORDINESS_PREMIUM_WITH_THE_INTENTION_OF)
[grammar] ~57-~57: Ensure spelling is correct
Context: ...peline engine between user-provided and dspo-managed). That is - if a DSPA has a WC...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~57-~57: In American English, abbreviations like “etc.” require a period.
Context: ...eir own ArgoWF, how are runs, versions, etc persisted between the two Argo Workflow...
(ETC_PERIOD)
[style] ~91-~91: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...h that said, for ODH installations with a large number of DSPAs it would be unsustainable to requ...
(LARGE_NUMBER_OF)
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_test_plan.md
59-59: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
167-167: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
173-173: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
180-180: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
|
Change to PR detected. A new PR build was completed. |
nsingla
force-pushed
the
bring-your-own-argo-workflow-testplan
branch
from
301e143 to
d55993a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
proposals/bring_your_own_argo_workflows/byoaw_context.md (1)
94-102: YAML indentation is still invalid –datasciencepipelinesmust be nested undercomponentsThe snippet will not parse as-YAML because
datasciencepipelines:is indented at the same level ascomponents:.
Readers copying this manifest will hit confusing validation errors.spec: components: - datasciencepipelines: - managementState: Managed - argoWorkflowsControllers: - managementState: Removed + datasciencepipelines: + managementState: Managed + argoWorkflowsControllers: + managementState: RemovedAdjust the remaining lines in the snippet (not shown here) to keep a consistent 2-space increment per level.
🧹 Nitpick comments (1)
proposals/bring_your_own_argo_workflows/byoaw_context.md (1)
21-22: Minor punctuation – add the terminating period after “etc.”Both occurrences of “etc” are missing the trailing period required in American English.
- * Includes Workflows, WorkflowTemplates, CronWorkflows, etc + * Includes Workflows, WorkflowTemplates, CronWorkflows, etc.- …run history/artifact lineage should be maintained. The documentation produced should mention these conditions, versions, etc persisted between the two Argo Workflows instances. + …run history/artifact lineage should be maintained. The documentation produced should mention these conditions, versions, etc. persisted between the two Argo Workflows instances.Purely cosmetic but keeps the doc polished.
Also applies to: 57-58
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proposals/bring_your_own_argo_workflows/byoaw_context.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
proposals/bring_your_own_argo_workflows/byoaw_context.md
[uncategorized] ~9-~9: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...his block. ## Feature Requirements ### High level requirements * As a Cluster Administrat...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ating a platform-level configuration. * As a Cluster Administrator I want to be ab...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~14-~14: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...that has Argo Workflows installation. * As a Cluster Administrator I want to manag...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~15-~15: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Workflow installation independently. * As a Cluster Administrator, I want to easi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: In American English, abbreviations like “etc.” require a period.
Context: ...lows, WorkflowTemplates, CronWorkflows, etc * Version of supported Argo Workflows, ...
(ETC_PERIOD)
[style] ~22-~22: This is not the usual sequence for adjectives that have no special emphasis.
Context: ...acked and tested for compatibility upon new minor releases * Example: ensure an ArgoWF ...
(EN_ADJ_ORDER)
[style] ~36-~36: In American English, abbreviations like “etc.” require a period.
Context: ...ipeline engine via k8s interfaces (CRs, etc). In turn, the Data Science Pipelines ...
(ETC_PERIOD)
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...pecially one that is deployed by a user outside of a Data Science Pipelines Application st...
(OUTSIDE_OF)
[style] ~38-~38: ‘with the intention of’ might be wordy. Consider a shorter alternative.
Context: ... they are working with this in mind and with the intention of introducing a written mandate that z-st...
(EN_WORDINESS_PREMIUM_WITH_THE_INTENTION_OF)
[style] ~57-~57: In American English, abbreviations like “etc.” require a period.
Context: ...eir own ArgoWF, how are runs, versions, etc persisted between the two Argo Workflow...
(ETC_PERIOD)
[style] ~91-~91: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...h that said, for ODH installations with a large number of DSPAs it would be unsustainable to requ...
(LARGE_NUMBER_OF)
nsingla
force-pushed
the
bring-your-own-argo-workflow-testplan
branch
from
d55993a to
0fc1086
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
proposals/bring_your_own_argo_workflows/byoaw_context.md (1)
98-103: Fix invalid YAML indentation –datasciencepipelinesmust be nested undercomponents
datasciencepipelines:is currently at the same level ascomponents:which makes the example manifest syntactically incorrect and will failoc apply.spec: components: - datasciencepipelines: - managementState: Managed - argoWorkflowsControllers: - managementState: Removed + datasciencepipelines: + managementState: Managed + argoWorkflowsControllers: + managementState: Removed
🧹 Nitpick comments (3)
proposals/bring_your_own_argo_workflows/byoaw_context.md (3)
78-90: Use explicit code-block language and remove the incidental “---” on the same line as the back-ticksThe current
```---pattern is treated as no language with stray dashes, so neither syntax-highlighting nor copy-pasting works cleanly.-```--- +```yaml +--- ... -```--- +```yaml +---Also applies to: 94-97
126-152: Conform unordered-list indentation to Markdown-lint (MD007) expectationsNested bullets are indented only two spaces instead of the required four (and some third-level items six instead of eight). This breaks consistent rendering and keeps the file from passing CI markdownlint.
Consider running
markdownlint --fixor apply a bulk shift right by two spaces for lines 126-152.
Example for the opening items:-* [Kubernetes Native Mode](https://github.com/kubeflow/pipelines/tree/master/proposals/11551-kubernetes-native-api) -* FIPS Mode -* Disconnected Cluster + * [Kubernetes Native Mode](https://github.com/kubeflow/pipelines/tree/master/proposals/11551-kubernetes-native-api) + * FIPS Mode + * Disconnected Cluster
163-167: Stick to a single bullet style (MD004) inside the test-case table instructionsThis small block mixes
-and+while the rest of the document uses*. Consistency avoids markdownlint noise:- - test case summary - - test steps - + Test steps should be a HTML format ordered list - - Expected results - + If there are multiple expectations, then it should be in a HTML format ordered list + * test case summary + * test steps + * Test steps should be an HTML ordered list + * Expected results + * If there are multiple expectations, list them as an HTML ordered list
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proposals/bring_your_own_argo_workflows/byoaw_context.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
proposals/bring_your_own_argo_workflows/byoaw_context.md
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ating a platform-level configuration. * As a Cluster Administrator I want to be ab...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~14-~14: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...that has Argo Workflows installation. * As a Cluster Administrator I want to manag...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~15-~15: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Workflow installation independently. * As a Cluster Administrator, I want to easi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: In American English, abbreviations like “etc.” require a period.
Context: ...lows, WorkflowTemplates, CronWorkflows, etc * Version of supported Argo Workflows, ...
(ETC_PERIOD)
[style] ~22-~22: This is not the usual sequence for adjectives that have no special emphasis.
Context: ...acked and tested for compatibility upon new minor releases * Example: ensure an ArgoWF ...
(EN_ADJ_ORDER)
[style] ~36-~36: In American English, abbreviations like “etc.” require a period.
Context: ...ipeline engine via k8s interfaces (CRs, etc). In turn, the Data Science Pipelines ...
(ETC_PERIOD)
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...pecially one that is deployed by a user outside of a Data Science Pipelines Application st...
(OUTSIDE_OF)
[style] ~38-~38: ‘with the intention of’ might be wordy. Consider a shorter alternative.
Context: ... they are working with this in mind and with the intention of introducing a written mandate that z-st...
(EN_WORDINESS_PREMIUM_WITH_THE_INTENTION_OF)
[style] ~57-~57: In American English, abbreviations like “etc.” require a period.
Context: ...eir own ArgoWF, how are runs, versions, etc persisted between the two Argo Workflow...
(ETC_PERIOD)
[style] ~91-~91: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...h that said, for ODH installations with a large number of DSPAs it would be unsustainable to requ...
(LARGE_NUMBER_OF)
[style] ~105-~105: In American English, abbreviations like “etc.” require a period.
Context: ...ing data, such as run details, metrics, etc) from an already-deployed DSPA. Curre...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_context.md
126-126: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
127-127: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
128-128: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
130-130: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
131-131: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
132-132: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
134-134: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
135-135: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
136-136: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
137-137: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
138-138: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
139-139: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
140-140: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
141-141: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
142-142: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
144-144: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
146-146: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
147-147: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
148-148: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
149-149: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
150-150: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
151-151: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
152-152: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
163-163: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
164-164: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
165-165: Unordered list style
Expected: asterisk; Actual: plus
(MD004, ul-style)
166-166: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
167-167: Unordered list style
Expected: asterisk; Actual: plus
(MD004, ul-style)
169-169: Bare URL used
(MD034, no-bare-urls)
173-173: Bare URL used
(MD034, no-bare-urls)
Signed-off-by: Nelesh Singla <[email protected]>
nsingla
force-pushed
the
bring-your-own-argo-workflow-testplan
branch
from
0fc1086 to
5088169
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
proposals/bring_your_own_argo_workflows/byoaw_context.md (1)
97-102: YAML snippet is still malformed –datasciencepipelinesmust be indented undercomponents.The current indentation produces an invalid CR and will mislead readers copying the example.
-spec: - components: - datasciencepipelines: - managementState: Managed - argoWorkflowsControllers: - managementState: Removed +spec: + components: + datasciencepipelines: + managementState: Managed + argoWorkflowsControllers: + managementState: Removed
🧹 Nitpick comments (5)
proposals/bring_your_own_argo_workflows/byoaw_context.md (5)
5-7: Replace “competition condition” with the correct term “contention condition”.“Competition condition” is not idiomatic; in concurrency and resource-conflict discussions the accepted term is “contention condition”.
- This would create a competition condition between the user-provided and DSP-provisioned AWF instances, + This would create a contention condition between the user-provided and DSP-provisioned AWF instances,
17-23: Minor language & punctuation fixes for clarity and consistency.– Compound adjective “High level” → “High-level”
– “etc” → “etc.” (requires period)-### High level requirements +### High-level requirements @@ - * Includes Workflows, WorkflowTemplates, CronWorkflows, etc + * Includes Workflows, WorkflowTemplates, CronWorkflows, etc.
126-152: Fix unordered-list indentation to satisfy markdownlint (MD007).The sub-lists are indented with 4 spaces; markdownlint expects 2. Adjusting keeps rendering consistent and silences CI lint jobs.
Example for the first few items:
-* [Kubernetes Native Mode](https://github.com/kubeflow/pipelines/tree/master/proposals/11551-kubernetes-native-api) -* FIPS Mode -* Disconnected Cluster + * [Kubernetes Native Mode](…) + * FIPS Mode + * Disconnected ClusterApply the same 2-space indent to all nested bullets in Lines 126-152.
163-167: Unify bullet marker style and add missing periods to expected-results list.markdownlint (MD004) flags mixed markers (
-,+). Use a single marker (asterisk*is project default) and terminate sentences with periods for consistency.- - test case summary - - test steps - + Test steps should be a HTML format ordered list - - Expected results - + If there are multiple expectations, then it should be in a HTML format ordered list + * Test case summary. + * Test steps. + * Test steps should be an HTML ordered list. + * Expected results. + * If there are multiple expectations, they should be in an HTML ordered list.
169-173: Convert bare URLs to reference links to satisfy markdownlint (MD034).-* https://github.com/kubeflow/pipelines/blob/c1876c509aca1ffb68b467ac0213fa88088df7e1/proposals/11551-kubernetes-native-api/TestPlan.md +* [KFP Kubernetes-native API test plan](https://github.com/kubeflow/pipelines/blob/c1876c509aca1ffb68b467ac0213fa88088df7e1/proposals/11551-kubernetes-native-api/TestPlan.md)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proposals/bring_your_own_argo_workflows/byoaw_context.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
proposals/bring_your_own_argo_workflows/byoaw_context.md
[uncategorized] ~9-~9: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...his block. ## Feature Requirements ### High level requirements * As a Cluster Administrat...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ating a platform-level configuration. * As a Cluster Administrator I want to be ab...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~14-~14: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...that has Argo Workflows installation. * As a Cluster Administrator I want to manag...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~15-~15: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Workflow installation independently. * As a Cluster Administrator, I want to easi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: In American English, abbreviations like “etc.” require a period.
Context: ...lows, WorkflowTemplates, CronWorkflows, etc * Version of supported Argo Workflows, ...
(ETC_PERIOD)
[style] ~22-~22: This is not the usual sequence for adjectives that have no special emphasis.
Context: ...acked and tested for compatibility upon new minor releases * Example: ensure an ArgoWF ...
(EN_ADJ_ORDER)
[style] ~36-~36: In American English, abbreviations like “etc.” require a period.
Context: ...ipeline engine via k8s interfaces (CRs, etc). In turn, the Data Science Pipelines ...
(ETC_PERIOD)
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...pecially one that is deployed by a user outside of a Data Science Pipelines Application st...
(OUTSIDE_OF)
[style] ~38-~38: ‘with the intention of’ might be wordy. Consider a shorter alternative.
Context: ... they are working with this in mind and with the intention of introducing a written mandate that z-st...
(EN_WORDINESS_PREMIUM_WITH_THE_INTENTION_OF)
[style] ~57-~57: In American English, abbreviations like “etc.” require a period.
Context: ...eir own ArgoWF, how are runs, versions, etc persisted between the two Argo Workflow...
(ETC_PERIOD)
[style] ~91-~91: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...h that said, for ODH installations with a large number of DSPAs it would be unsustainable to requ...
(LARGE_NUMBER_OF)
[style] ~105-~105: In American English, abbreviations like “etc.” require a period.
Context: ...ing data, such as run details, metrics, etc) from an already-deployed DSPA. Curre...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
proposals/bring_your_own_argo_workflows/byoaw_context.md
126-126: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
127-127: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
128-128: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
130-130: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
131-131: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
132-132: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
134-134: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
135-135: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
136-136: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
137-137: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
138-138: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
139-139: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
140-140: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
141-141: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
142-142: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
144-144: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
146-146: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
147-147: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
148-148: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
149-149: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
150-150: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
151-151: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
152-152: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
163-163: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
164-164: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
165-165: Unordered list style
Expected: asterisk; Actual: plus
(MD004, ul-style)
166-166: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
167-167: Unordered list style
Expected: asterisk; Actual: plus
(MD004, ul-style)
169-169: Bare URL used
(MD034, no-bare-urls)
173-173: Bare URL used
(MD034, no-bare-urls)
|
Change to PR detected. A new PR build was completed. |
1 similar comment
|
Change to PR detected. A new PR build was completed. |
The test plan is to cover different testing that is require to ensure that BYO AW works as expected and does not cause any regression. Some of the tests listed may not be worth automating and we probably need to perform those only 1 time
Checklist:
Summary by CodeRabbit