Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Client Design changes #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

agourdel
Copy link

Design changes proposal

First of all, I would like to say it was a pleasure to work on a clear and fresh code.

The changes concern mainly the CLIENT part of the CLI.
The changes in CMD are only adaptions to changes in the CLIENT side.

Structural changes for the CLIENT part

  • Client agnosticity

    There was a blurred boundary between the client concerns and the CLI concerns.
    The client can be used in different software contexts so it appears to me it was important to rearrange that.
    Profiles management and loading ENV variables have been removed from the CLIENT and were entrusted to the CLI part.
    In result, the instanciation of the client is way more easier.

      clientOptions := client.GetDefaultClientOptions()
      clientOptions.usePasswordAuth(email, password) // or clientOptions.useTokenAuth(orgaID, token)
    
      ApiClient := client.New(clientOptions)
    
  • Structuring the client's lower layer

    With the aim of facilitating future developments and complexity, both horizontal and vertical,
    a separation has been created between the API server logic part and the client's useCases part.
    A folder /api has been created with two files.
    - profile.go
    - request.go
    The first one encapsulates the profile of all the endpoints of the AppClacks's API

      { ... }
      
      var healthCheckCmdCreate *endPoint = &endPoint{
        Name : "HealthCheckCmdCreate",
        Uri : V1_HC_COMMAND_URI,
        Method: METH_POST,
        Suffix: "",
        }
      var healthCheckCmdUpdate *endPoint = &endPoint{
        Name : "HealthCheckCmdUpdate",
        Uri : V1_HC_COMMAND_URI,
        Method : METH_PUT,
        Suffix: "/%s",
      }
      
      {...}
    

    The second one concerns a RequestHelper which is the struct systematically called by the
    CLIENT's UseCases part. Each EndPoint has its own buildRequest method.

  • ** Standardization of use cases **

    As a result of the previous point, all the useCases have now the exact same syntax structures.

    1. Controlling args and situation
    2. Creating request with the HelperRequest
    3. Consume request and send back result.

./client/healthcheck_command.go

    {...}
    
    func (c *Client) CreateCommandHealthcheck(input apitypes.CreateCommandHealthcheckInput) (apitypes.Healthcheck, error) {
        var result apitypes.Healthcheck
        
        if !c.Credentials.HasAuth() {
            return apitypes.Healthcheck{}, formatError("CreateCommandHealthcheck", "Credentials missing")
        }
        
        request, err := c.RequestsHelper.BuildCreateCommandHealthCheckRequest(input)
        if err != nil {
            return apitypes.Healthcheck{}, err 
        }
        
        return sendRequestGetStruct(c.RequestsHelper, request, result)
    }
    
    {...}

In conclusion, the customer side should now be able to welcome new features more easily

New features for the client part

  • All the useCases of the client are now compatibles with both Auth and Password authorization mode.
  • All the useCases of the client can be called with or without a context.Context object.

Tests Results

  • The routes /v1/healthcheck, /v1/result and /v1/metrics do not exist server side with Password authorization mode. (With the prefix /app/ we got "Not found, 404"
  • The route /v1/organization doesn't exist with Token authorization mode. (With the prefix /api/ we got "Not found, 404)
  • The route `/api/v1/result/healthchecks' send back an error 500.
  • Cabourotte Discovery hasn't been tested.

Notes, Remarks and Questions about the global CLI

  • About the "github.com/appclacks/go-types" :

    • Why have the common parts of the different Healthecks type been hyperdistributed while the specificities one have been factorized?
    • Why the ListHealthchecksResultsInput.Success is a boolean ptr instead of boolean ?
    • For Healthcheck types, labels are map[string]string but for the Cabourotte Discovery useCases, labels is just one string.
  • About the Api Client :

    • The HealthchecksXXX calls send back Healthcheck type and not HealthcheckXXX type
    • There is no command to enable Healthcheck ???
  • About the CLI :

    • For the Organization creation process, shouldn't be more easy for the user to let him create an account and the organization is automatically created instead of ask him to create organization AND account ?
    • The APi's errors should be intrepreted by the CLI part and not sent back to the user. ("The API returned an error: status 403 {"messages":["Forbidden"]}" should be "Wrong Email/Password"}
    • I think the Healthcheck creation process should be rewrite. I feel like it's really painfull from user point of view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant