Skip to content

Conversation

@Dannick-debug
Copy link
Member

Pull Request Description

Objective

This pull request introduces functions to facilitate the use of the BTP CLI. The key functionalities added include:

  • Connecting to the BPI Cockpit (login/logout)
  • Creating, retrieving, and deleting service instances
  • Creating, retrieving, and deleting service bindings

Key Improvements

  • CommandBuilder Implementation: A CommandBuilder has been developed to streamline script creation, ensuring efficient command handling.
  • Execution Mechanisms: Two execution mechanisms have been introduced:
    • Synchronous Execution (Run): Enables concurrent task execution without blocking the main process.
    • Asynchronous Execution (RunSync): Incorporates a polling mechanism to verify the success of BTP actions at regular intervals.

Synchronous Execution

Currently, BTP actions do not support direct synchronous execution. To address this, a polling mechanism has been implemented to continuously check the status of actions, ensuring they are completed successfully.

Checklist

  • Tests: Comprehensive tests have been conducted to ensure functionality and reliability.

@cla-assistant
Copy link

cla-assistant bot commented Sep 8, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@nayyer28 nayyer28 left a comment

Choose a reason for hiding this comment

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

lgtm!

/*
@param timeout : in seconds
@param pollInterval : in seconds
@param negativeCheck : set to false if you whant to check the negation of the response of `cmdCheck`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@param negativeCheck : set to false if you whant to check the negation of the response of `cmdCheck`
@param negativeCheck : set to false if you want to check the negation of the response of `cmdCheck`

@DanielMieg
Copy link
Member

Maybe you could consider some naming changes. I noticed the following things:

  • btp_test.go tests executor.go
  • The mock is called btp_runner.go but mocks the "ExecRunner"
  • Some files use camel case and some use snake case (not concerning the _test suffix)
  • If a function returns a boolean, the function is (with some naming conventions) named as a question:

CheckServiceInstanceCreated -> IsServiceInstanceCreated

Could you add unit tests for the rest of the files? (checkFunctions.go & command_builder.go). Maybe that was your plan anyway, as this is still a draft

@@ -0,0 +1,118 @@
package btputils
Copy link
Member

Choose a reason for hiding this comment

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

Looks good 👍. One small thing: I would move structures.go to the btp package.I think it does not make sense to create a whole package just for some structures

Copy link
Member

@DanielMieg DanielMieg left a comment

Choose a reason for hiding this comment

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

see comment

@Dannick-debug Dannick-debug marked this pull request as ready for review September 30, 2025 13:28
@Dannick-debug Dannick-debug requested a review from a team as a code owner September 30, 2025 13:28
@sonarqubecloud
Copy link

@DanielMieg DanielMieg requested a review from o-liver September 30, 2025 13:29
@DanielMieg DanielMieg requested a review from tiloKo October 13, 2025 13:43
@DanielMieg DanielMieg changed the title Replace cf cli with btp api Add BTP CLI Oct 13, 2025
@c0dev0id
Copy link

c0dev0id commented Oct 15, 2025

We were just about to request an option to create service bindings/instances on the BTP without CF and then found this PR.
Thanks for the work! (sorry, can't comment using my work account for some reason...)

if len(b.params) > 0 {
cmd += " " + strings.Join(b.params, " ")
}
return cmd, nil
Copy link
Member

Choose a reason for hiding this comment

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

I see this command is put together with spaces. How is this executed? Not in a shell right? As I know it we should pass these as a list to an executor, to prevent command injections. Check other such commands e.g. https://github.com/SAP/jenkins-library/blob/master/pkg/npm/npm.go#L129

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right.
Now we will only use Array of string in the builder (no more concatenations)

Copy link
Member

@o-liver o-liver left a comment

Choose a reason for hiding this comment

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

Looks good to me. Only minor suggestions made, take it or leave.

Comment on lines 37 to 40
"btp check": `dummy
dummy

OK`,
Copy link
Member

Choose a reason for hiding this comment

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

why such a weird string?

Suggested change
"btp check": `dummy
dummy
OK`,
"btp check": "OK",

Copy link
Member Author

Choose a reason for hiding this comment

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

corrected 👍🏾

Comment on lines 28 to 36
StdoutReturn: map[string]string{
"btp login .*": "Authentication successful",
"btp get services/binding": fmt.Sprintf(`
{
"id": "xxxx",
"name": "%s",
"ready": true
}`, btpConfig.BindingName),
},
Copy link
Member

Choose a reason for hiding this comment

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

this json some extra spaces should not matter:

Suggested change
StdoutReturn: map[string]string{
"btp login .*": "Authentication successful",
"btp get services/binding": fmt.Sprintf(`
{
"id": "xxxx",
"name": "%s",
"ready": true
}`, btpConfig.BindingName),
},
StdoutReturn: map[string]string{
"btp login .*": "Authentication successful",
"btp get services/binding": fmt.Sprintf(`
{
"id": "xxxx",
"name": "%s",
"ready": true
}`, btpConfig.BindingName),
},

Copy link
Member Author

Choose a reason for hiding this comment

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

corrected 👍🏾

Comment on lines 73 to 77
{
"id": "xxxx",
"name": "%s",
"ready": true
}`, btpConfig.BindingName),
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Member Author

Choose a reason for hiding this comment

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

corrected 👍🏾

Comment on lines 119 to 122
{
"error": "BadRequest",
"description": "Could not find such binding"
}`),
Copy link
Member

Choose a reason for hiding this comment

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

again a json string, which could be indented nicer

Copy link
Member Author

Choose a reason for hiding this comment

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

corrected 👍🏾

@DanielMieg
Copy link
Member

/it

@o-liver
Copy link
Member

o-liver commented Nov 5, 2025

/it-go

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 5, 2025

@DanielMieg DanielMieg merged commit a8e29db into SAP:master Nov 5, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants