Update run command based on new cpln changes
#182
Conversation
WalkthroughThe recent changes enhance the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant CLI
participant ControlPlane
User->>CLI: Execute `run` command with overrides
CLI->>ControlPlane: Validate command
ControlPlane-->>CLI: Validation result
alt Validation Passes
CLI->>ControlPlane: Set default or overridden image, CPU, memory
ControlPlane-->>CLI: Confirmation
CLI->>ControlPlane: Start job
ControlPlane-->>CLI: Job started
else Validation Fails
CLI-->>User: Error message
end
CLI-->>User: Job status
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
rafaelgomesxyz
force-pushed
the
run-cpln-changes
branch
2 times, most recently
from
1ce60c4 to
63db167
Compare
rafaelgomesxyz
force-pushed
the
run-cpln-changes
branch
from
63db167 to
1996772
Compare
rafaelgomesxyz
changed the title
run command based on new cpln changesrun command based on new cpln changes
![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
Outside diff range and nitpick comments (5)
docs/commands.md (3)
Line range hint
44-44: Consider revising the phrase "amount of days" to "number of days" for grammatical accuracy.- Specify the amount of days through `image_retention_days` in the `.controlplane/controlplane.yml` file + Specify the number of days through `image_retention_days` in the `.controlplane/controlplane.yml` file
Line range hint
56-56: Consider revising the phrase "amount of days" to "number of days" for grammatical accuracy.- Specify the amount of days after an app should be considered stale through `stale_app_image_deployed_days` in the `.controlplane/controlplane.yml` file + Specify the number of days after an app should be considered stale through `stale_app_image_deployed_days` in the `.controlplane/controlplane.yml` file
Line range hint
230-230: Consider using a hyphen in "path based" to form the compound adjective "path-based."- path based routing mode + path-based routing modeAlso applies to: 241-241, 252-252
lib/command/run.rb (1)
Line range hint
89-100: The initialization of instance variables in thecallmethod is well-structured. However, consider adding a brief comment explaining the purpose of each variable for better code readability.+ # Initialize instance variables with configuration options and defaults @interactive = config.options[:interactive] || interactive_command? @detached = config.options[:detached] @log_method = config.options[:log_method] @location = config.location @original_workload = config.options[:workload] || config[:one_off_workload] @runner_workload = "#{original_workload}-runner" @default_image = cp.latest_image_from([], app_name: config.app)CHANGELOG.md (1)
Line range hint
143-143: Correct the spelling of "macOS".- Fixed failed build on MacOS by adding platform flag and fixed multiple files in yaml document for template. + Fixed failed build on macOS by adding platform flag and fixed multiple files in yaml document for template.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- docs/commands.md (1 hunks)
- lib/command/run.rb (9 hunks)
Additional Context Used
LanguageTool (28)
CHANGELOG.md (18)
Near line 26: You might be missing the article “the” here.
Context: ...## Added - Added post-creation hook tosetup-appcommand (configurable through...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 35: To form a complete sentence, be sure to include a subject.
Context: ... non-zero code if any validation fails. Can be disabled by setting `DISABLE_VALIDAT...
Rule ID: MISSING_IT_THERE
Near line 46: You might be missing the article “the” here.
Context: ...4-05-15 ### Fixed - Fixed issue wherecleanup-stale-appscommand fails to del...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 63: You might be missing the article “the” here.
Context: ...mesxyz). ### Added - Added options toruncommand to override the workload co...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 64: You might be missing the article “the” here.
Context: ...mesxyz). - Added--workloadoption todeletecommand to delete a specific wor...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 65: You might be missing the article “the” here.
Context: ...omesxyz). - Added--replicaoption tologscommand to see logs from a specifi...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 75: You might be missing the article “the” here.
Context: ...y-image` command now raises an error if image does not exist. [PR 153](https://github...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 98: You might be missing the article “the” here.
Context: ..._app_name_starts_withset totrueincopy-image-from-upstream` command. [PR 1...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 110: You might be missing the article “the” here.
Context: ... over config fromcontrolplane.ymlincopy-image-from-upstreamcommand. [PR 1...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 121: You might be missing the article “the” here.
Context: ...hub.com/ahangarha). - Fixed issue wheredeletecommand fails to delete apps wit...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 143: The operating system from Apple is written “macOS”.
Context: ...-17 ### Fixed - Fixed failed build on MacOS by adding platform flag and fixed multi...
Rule ID: MAC_OS
Near line 147: You might be missing the article “the” here.
Context: ...ole` command to open the app console on Control Plane. [PR 83](https://github.com/shaka...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 165: You might be missing the article “the” here.
Context: ...3-09-20 ### Fixed - Fixed issue wherecopy-image-from-upstreamcommand does n...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 183: You might be missing the article “an” here.
Context: ...runcommands fail when not providing image. [PR 68](https://github.com/shakacode/c...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_AN
Near line 191: You might be missing the article “the” here.
Context: ...s://github.com/rafaelgomesxyz). - Fixedrun:cleanupcommand for all apps that s...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 192: You might be missing the article “the” here.
Context: ...s://github.com/rafaelgomesxyz). - Fixedcleanup-old-imagescommand for all apps...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 193: You might be missing the article “the” here.
Context: ...s://github.com/rafaelgomesxyz). - Fixed--helpoption. [PR 66](https://github.c...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 197: You might be missing the article “the” here.
Context: ... - Added--use-local-tokenoption torun:detachedcommand. [PR 61](https://g...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THEdocs/commands.md (10)
Near line 44: You might be missing the article “the” here.
Context: ...mage` - Builds and pushes the image to Control Plane - Automatically assigns image num...
Rule ID: AI_EN_LECTOR_MISSING_DETERMINER_THE
Near line 56: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...uantity or are older than the specified amount of days - Specify the max quantity thro...
Rule ID: AMOUNTOF_TO_NUMBEROF
Near line 57: Possible verb agreement error. Did you mean “specifies”? (Some collective nouns can be treated as both singular and plural, so ‘Specify’ is not always incorrect.)
Context: ...der than the specified amount of days - Specify the max quantity through `image_retenti...
Rule ID: COLLECTIVE_NOUN_VERB_AGREEMENT_VBP
Near line 58: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ne/controlplane.ymlfile - Specify the amount of days throughimage_retention_days` ...
Rule ID: AMOUNTOF_TO_NUMBEROF
Near line 72: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ... date of the latest image - Specify the amount of days after an app should be consider...
Rule ID: AMOUNTOF_TO_NUMBEROF
Near line 230: This expression is usually spelled with a hyphen.
Context: ... is only supported for domains that use path based routing mode and have a route configure...
Rule ID: BASED_HYPHEN
Near line 241: This expression is usually spelled with a hyphen.
Context: ... is only supported for domains that use path based routing mode and have a route configure...
Rule ID: BASED_HYPHEN
Near line 252: This expression is usually spelled with a hyphen.
Context: ... is only supported for domains that use path based routing mode and have a route configure...
Rule ID: BASED_HYPHEN
Near line 376: Unpaired symbol: ‘"’ seems to be missing
Context: ...t isbash, in which case the args ["-c", cmd_to_run] are passed - Providing `--...
Rule ID: EN_UNPAIRED_QUOTES
Near line 428: Consider replacing ‘only’ with a different word to let your writing stand out.
Context: ...ne/controlplane.yml` file - This should only be used for temporary apps like review ...
Rule ID: ONLY_EXCLUSIVELY_STYLE
Markdownlint (2)
docs/commands.md (2)
5: null
Fenced code blocks should have a language specified
23: null
Fenced code blocks should have a language specified
Additional comments not posted (7)
docs/commands.md (1)
374-379: Ensure the quotation marks are correctly paired in the documentation.Verification successful
The verification confirms that all quotation marks in the specified lines of the
docs/commands.mdfile are correctly paired.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct pairing of quotation marks in the documentation. # Test: Search for unpaired quotation marks. Expect: No occurrences. rg --type markdown $'\"[^"]*\"' docs/commands.mdLength of output: 541
lib/command/run.rb (5)
39-44: The documentation for the--entrypointoption is clear and well-explained. It correctly specifies how the entrypoint can be overridden and the default behavior when set tonone.
158-160: Setting the image to the default image in thecreate_runner_workloadmethod is a good practice as it ensures consistency across jobs. This change helps in managing different job configurations effectively.
179-187: The logic to check and update the image in theupdate_runner_workloadmethod is clear and concise. It ensures that the workload is always running with the correct image, which is crucial for the reliability of the system.
436-437: The conditional check before printing detached commands is a good safeguard against unnecessary output, especially in scenarios where the replica might not be set up yet.
455-455: Handling both "active" and "pending" statuses in theresolve_job_statusmethod is a robust way to ensure that the job status is accurately tracked. This change addresses potential issues with job initialization delays.CHANGELOG.md (1)
34-34: Ensure clarity in documentation.Consider rephrasing to form a complete sentence for better readability:
- Can be disabled by setting `DISABLE_VALIDATIONS` env var to `true`. + This can be disabled by setting the `DISABLE_VALIDATIONS` environment variable to `true`.Likely invalid or redundant comment.
rafaelgomesxyz
force-pushed
the
run-cpln-changes
branch
from
1996772 to
332dec7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
Line range hint
146-146: Correct the spelling of "macOS" to maintain consistency with the official brand name.- Fixed failed build on MacOS by adding platform flag and fixed multiple files in yaml document for template. + Fixed failed build on macOS by adding platform flag and fixed multiple files in yaml document for template.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional Context Used
LanguageTool (2)
CHANGELOG.md (2)
Near line 38: To form a complete sentence, be sure to include a subject.
Context: ... non-zero code if any validation fails. Can be disabled by setting `DISABLE_VALIDAT...
Rule ID: MISSING_IT_THERE
Near line 146: The operating system from Apple is written “macOS”.
Context: ...-17 ### Fixed - Fixed failed build on MacOS by adding platform flag and fixed multi...
Rule ID: MAC_OS
Additional comments not posted (1)
CHANGELOG.md (1)
19-19: The entry for theruncommand update is clear and accurately reflects the changes made in PR 182. It's well-linked to the PR and the contributor, enhancing traceability.
rafaelgomesxyz
force-pushed
the
run-cpln-changes
branch
from
332dec7 to
2133ff4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (9)
CHANGELOG.md (2)
Line range hint
147-147: Correct the spelling of "MacOS" to "macOS" for consistency with Apple's branding.- Fixed failed build on MacOS by adding platform flag and fixed multiple files in yaml document for template. + Fixed failed build on macOS by adding platform flag and fixed multiple files in yaml document for template.
Line range hint
39-39: Consider rephrasing to form a complete sentence for clarity.- Can be disabled by setting `DISABLE_VALIDATIONS` env var to `true`. + This can be disabled by setting the `DISABLE_VALIDATIONS` environment variable to `true`.README.md (7)
Line range hint
36-36: Consider using a synonym for "gives" to enhance the writing style.- Control Plane, on the other hand, gives you access to raw cloud computing power. + Control Plane, on the other hand, provides access to raw cloud computing power.
Line range hint
76-76: Rephrase to avoid repetitive sentence beginnings.- has common environment variables. + and includes common environment variables.
Line range hint
113-113: Rephrase to avoid repetitive sentence beginnings.- Install Control Plane CLI, and configure access... + Next, install the Control Plane CLI and configure access...
Line range hint
160-160: Correct the verb form for "setup".- which could setup Redis and Postgres for a testing application. + which could set up Redis and Postgres for a testing application.
Line range hint
382-382: Add a hyphen to "open source" when used as a compound adjective.- for ShakaCode open source projects. + for ShakaCode open-source projects.
Line range hint
430-430: Add an article before "database setup".- several options for a database setup on Control Plane: + several options for a database setup on the Control Plane:
Line range hint
440-440: Add a comma after "e.g." for clarity.- e.g., Amazon's RDS can be a quick go-to. + e.g., Amazon's RDS, can be a quick go-to.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- README.md (1 hunks)
- docs/commands.md (1 hunks)
- examples/controlplane.yml (1 hunks)
- lib/command/run.rb (8 hunks)
Additional context used
LanguageTool
docs/commands.md
[uncategorized] ~56-~56: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...uantity or are older than the specified amount of days - Specify the max quantity thro...
[grammar] ~57-~57: Possible verb agreement error. Did you mean “specifies”? (Some collective nouns can be treated as both singular and plural, so ‘Specify’ is not always incorrect.)
Context: ...der than the specified amount of days - Specify the max quantity through `image_retenti...
[uncategorized] ~58-~58: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ne/controlplane.ymlfile - Specify the amount of days throughimage_retention_days` ...
[uncategorized] ~72-~72: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ... date of the latest image - Specify the amount of days after an app should be consider...
[uncategorized] ~230-~230: This expression is usually spelled with a hyphen.
Context: ... is only supported for domains that use path based routing mode and have a route configure...
[uncategorized] ~241-~241: This expression is usually spelled with a hyphen.
Context: ... is only supported for domains that use path based routing mode and have a route configure...
[uncategorized] ~252-~252: This expression is usually spelled with a hyphen.
Context: ... is only supported for domains that use path based routing mode and have a route configure...
[typographical] ~376-~376: Unpaired symbol: ‘"’ seems to be missing
Context: ...t isbash, in which case the args ["-c", cmd_to_run] are passed - Providing `--...
[style] ~431-~431: Consider replacing ‘only’ with a different word to let your writing stand out.
Context: ...ne/controlplane.yml` file - This should only be used for temporary apps like review ...CHANGELOG.md
[style] ~39-~39: To form a complete sentence, be sure to include a subject.
Context: ... non-zero code if any validation fails. Can be disabled by setting `DISABLE_VALIDAT...
[grammar] ~147-~147: The operating system from Apple is written “macOS”.
Context: ...-17 ### Fixed - Fixed failed build on MacOS by adding platform flag and fixed multi...README.md
[style] ~36-~36: Try using a synonym here to strengthen your writing.
Context: ...ons. Control Plane, on the other hand, gives you access to raw cloud computing power. Ho...
[style] ~76-~76: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ding the database and other services. - has common environment variables. On Contr...
[style] ~113-~113: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .../en/) (required for these helpers). 4. Install Control Plane CLI, and configure access...
[grammar] ~160-~160: The word “setup” is a noun. The verb is spelled with a space.
Context: ...is.ymlandpostgres.yml`, which could setup Redis and Postgres for a testing applic...
[uncategorized] ~382-~382: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...yYWQ3MzA2NGE1YWJjNmVlMGE) for ShakaCode open source projects. ## Environment There are tw...
[uncategorized] ~430-~430: Possible missing article found.
Context: ...several options for a database setup on Control Plane: - Heroku Postgres. It is th...
[uncategorized] ~440-~440: Possible missing comma found.
Context: ...d provider for Postgres, e.g., Amazon's RDS can be a quick go-to. Here are [instr...
Markdownlint
docs/commands.md
5-5: null
Fenced code blocks should have a language specified
23-23: null
Fenced code blocks should have a language specifiedREADME.md
540-540: Expected: dash; Actual: asterisk
Unordered list style
541-541: Expected: dash; Actual: asterisk
Unordered list style
542-542: Expected: dash; Actual: asterisk
Unordered list style
14-14: Expected: 1; Actual: 2
Multiple consecutive blank lines
539-539: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
540-540: null
Lists should be surrounded by blank lines
21-21: Expected: ----; Actual: ---
Horizontal rule style
28-28: Expected: ----; Actual: ---
Horizontal rule style
128-128: null
Spaces inside code span elements
143-143: null
Fenced code blocks should have a language specified
Additional comments not posted (3)
examples/controlplane.yml (1)
89-93: The updated default CPU and memory settings align with the PR objectives and should help in managing resources more effectively.Verification successful
The new default settings for
runner_job_default_cpuandrunner_job_default_memoryare correctly integrated and utilized in the codebase.
lib/command/run.rb: The settings are used to set default CPU and memory values for jobs.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new default settings are correctly utilized in job configurations. # Test: Search for the usage of `runner_job_default_cpu` and `runner_job_default_memory` in job configurations. rg --type yaml $'runner_job_default_cpu|runner_job_default_memory'Length of output: 188
Script:
#!/bin/bash # Description: Verify the integration of `runner_job_default_cpu` and `runner_job_default_memory` in the codebase. # Test: Search for the usage of `runner_job_default_cpu` and `runner_job_default_memory` in the codebase. rg 'runner_job_default_cpu|runner_job_default_memory'Length of output: 1065
lib/command/run.rb (1)
Line range hint
166-207: The implementation for handling default settings and entrypoint overrides in theruncommand is well-done. It should ensure that the command behaves as expected with the new configurations.CHANGELOG.md (1)
19-20: The updates to theruncommand in the CHANGELOG are clear and well-documented.
Fixes #164
cplnnow accepts overridingimage,cpu, andmemorywhen starting a Cron workload job, so we no longer have to worry about race conditions when running simultaneous jobs with different overrides.This PR updates the
runcommand to reflect those changes.Also changes jobs to use a CPU size of 1 (1 core) and a memory size of 2Gi (2 gibibytes) by default (configurable through
runner_job_default_cpuandrunner_job_default_memoryincontrolplane.yml), as just copying the sizes from the original workload may lead to an overprovision of resources.Summary by CodeRabbit
New Features
--image,--cpu, and--memoryfor each job individually in theruncommand.Improvements
--skip-secret-access-bindingoption with--skip-secrets-setup.Documentation
--entrypointand terminal size configuration options.