Skip to content

Conversation

@Dannick-debug
Copy link
Member

@Dannick-debug Dannick-debug commented Mar 27, 2025

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 Mar 27, 2025

CLA assistant check
All committers have signed the CLA.

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.

Reviewed Authentication.go - will do the rest later.
I gave a little bit of feedback regarding the structure. It would be good to think about the loggedIn boolean again.

@DanielMieg DanielMieg requested a review from doldsimo March 31, 2025 09:53
@DanielMieg DanielMieg requested review from a team, o-liver and tiloKo April 28, 2025 13:32
@Dannick-debug Dannick-debug changed the title Replace cf cli with btp api Add methods to use the BTP CLI Apr 30, 2025
@Dannick-debug Dannick-debug marked this pull request as ready for review April 30, 2025 07:47
@Dannick-debug Dannick-debug requested a review from a team as a code owner April 30, 2025 07:47
@o-liver o-liver requested a review from srinikitha09 April 30, 2025 09:43
@o-liver
Copy link
Member

o-liver commented Apr 30, 2025

So this is a fist step, right? And then in another PR you will introduce the corresponding steps that consume this, or put an option into the cf deploy, and other cf steps, to use btp cli instead of cf cli?

@o-liver
Copy link
Member

o-liver commented Apr 30, 2025

Currently we provide the cf-cli with this docker image: https://github.com/SAP/devops-docker-cf-cli

Will you provide a docker image for the btp cli as well, or what is the plan?

@Dannick-debug
Copy link
Member Author

So this is a fist step, right? And then in another PR you will introduce the corresponding steps that consume this, or put an option into the cf deploy, and other cf steps, to use btp cli instead of cf cli?

Yes exactly it is the first step. In another PR we will then:

  • introduce the consumption of these functions in the different steps.
  • And also introduce an additional option in the different steps to choose between CF or BTP.

@Dannick-debug
Copy link
Member Author

Currently we provide the cf-cli with this docker image: https://github.com/SAP/devops-docker-cf-cli

Will you provide a docker image for the btp cli as well, or what is the plan?

Thank you for bringing this up! Currently, our focus is on delivering the essential components in Golang for the BTP CLI. While we aren't providing a Docker image ourselves, team members who utilize these functions or steps will have the ability to create their own custom Docker environment if required.

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.

Could you change the filenames to lowercase? This seems to be the standard in this repo (the cloudfoundry folder seems to be an exception)

@Dannick-debug
Copy link
Member Author

Could you change the filenames to lowercase? This seems to be the standard in this repo (the cloudfoundry folder seems to be an exception)

I renamed some of the file to lower case 👍🏾

PollInterval int
}

type CreateServiceInstanceOptions struct {
Copy link
Member

Choose a reason for hiding this comment

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

There seem to be two parameters, "ServiceInstance" and "InstanceName". Are they the same? Could you check if the naming is consistent? Also, in regard to other types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes they where the same. I removed "ServiceInstance". Thank you for bringing it to my attention.

@sonarqubecloud
Copy link

@param pollInterval : in seconds
@param negativeCheck : set to false if you whant to check the negation of the response of `cmdCheck`
*/
func (e *Executor) RunSync(cmdScript string, cmdCheck string, timeout int, pollInterval int, negativeCheck bool) (err error) {
Copy link
Member

Choose a reason for hiding this comment

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

The error in err = e.Run(cmdScript) is unhandled. Also, the loop for time.Since(startTime) < timeoutDuration may not terminate in some conditions, due to
if (negativeCheck && (err != nil)) || (!negativeCheck && (err == nil)) always being false. Maybe the two points are related.

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 in executor.go

@DanielMieg DanielMieg closed this May 14, 2025
@DanielMieg
Copy link
Member

Follow-up in a different PR

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.

3 participants