Post json using http and verify whether all actions completed successfully or not

Question

I am working on a project where I need to work with http apis and call them using Python language. Below is what I need to do:

• Take few input parameters like - environmentName, instanceName, configName.
• Call get API on a service. Get the JSON data and compare the configName it already has vs what you want to update from the command line.
• If config is same then don't do anything and return back with a message.
• But if config is different then post new JSON data with new configName passed from command line in it to the service using http api. In this there are two things:
  • We will make new JSON with new configName in it along with one more key which is action and value of that will be download. After successfully posting this new JSON then we will call another api (verifyApi) to verify whether all machines have downloaded successfully that config or not. If they downloaded successfully then we will move to the next step otherwise we will fail and return.
  • If all machines downloaded successfully, then we will change action key to verify in the same JSON and then we will post it again. After successfully posting this new JSON, we will call same verifyApi to make sure all machines have verified successfully.

Once everything is successful return, otherwise fail it. Below is my code which does the job:

import requests
import json
import sys
import base64

# define all constants
actions=['download', 'verify']
endpoint="http://consul.{}.demo.com/"
config_path="v1/kv/app-name/{}/config"
status_path="v1/kv/app-name/{}/status/{}"
catalog_path="v1/catalog/service/app-name?ns=default&tag={}"
raw="?raw"

def update( url, json ):
    """
    This will make a PUT api call to update with new json config.
    """
    try:
        r = requests.put(url, data=json, headers={'Content-type': 'application/json'})
        r.raise_for_status()
        return True
    except requests.exceptions.HTTPError as errh:
        print ("Http Error:",errh)
    except requests.exceptions.ConnectionError as errc:
        print ("Error Connecting:",errc)
    except requests.exceptions.Timeout as errt:
        print ("Timeout Error:",errt)
    except requests.exceptions.RequestException as err:
        print ("OOps: Something Else",err)

    return False

def check( environment, instance, config, action ):
    """
    This will get list of all ipAddresses for that instance in that environment.
    And then check whether each machine from that list have downloaded and verified successfully.
    Basically for each 'action' type I need to verify things.
    If successful at the end then print out the message.
    """
    flag = True
    catalog_url = endpoint.format(environment) + catalog_path.format(instance)
    response = requests.get(catalog_url)
    url = endpoint.format(environment) + status_path.format(instance) + raw
    json_array = response.json()
    count = 0
    for x in json_array:
        ip = x['Address']
        count++
        r = requests.get(url.format(ip))
        data = r.json()
        if action == 'download' and "isDownloaded" in data and data['isDownloaded'] and "currentCfg" in data and data['currentCfg'] == config:
            continue
        elif action == 'verify' and  "activeCfg" in data and data['activeCfg'] == config:
            continue
        else:
            flag = False #set to False if above two if statements fail
            print("failed to " +action+ " on " +ip)

    if flag and action == 'download':
        print("downloaded successfully on all "+count+" machines")
    elif flag and action == 'verify':
        print("verified successfully on all "+count+" machines")

    return flag

def main():
    # capture inputs from command line
    environment = sys.argv[1]
    instance = sys.argv[2]
    new_config = sys.argv[3]

    # make url for get api to verify whether configs are same or not
    # as mentioned in point 1.
    url=endpoint.format(environment) + config_path.format(instance)
    response = requests.get(url + raw)
    data = json.loads(response.content)
    remote_config = data['remoteConfig']
    # compare remote_config in the json with config passed from command prompt
    if remote_config == new_config:
        print("cannot push same config")
        return

    # config is different
    data['remoteConfig'] = new_config
    # now for each action, update json and then verify whether that action completed successfully.
    for action in actions:
        data['action'] = action
        if update(url, json.dumps(data)):
            if check(environment, instance, new_config, action):
                continue
            else:
                print(action+ " action failed")
                return
        else:
            print("failed to update")
            return


if __name__ == "__main__":
    try:
        sys.exit(main())
    except KeyboardInterrupt:
        print("\nCaught ctrl+c, exiting")

Problem Statement

I have my above code which works fine so opting for code review to see if there is any better way to do the above things efficiently. I am also sure we can rewrite the above code in a much cleaner manner compared to the way I have since I am relatively new to Python and may have made a lot of mistakes in designing it. Given this code will be running in production wanted to see what can be done to improve it.

The idea is something like this when I run it from the command line.

If everything is successful then it should look like this:

python test.py dev master-instace test-config-123.tgz
downloaded successfully on all 10 machines
verified successfully on all 10 machines
config pushed successfully!!

In case of download action failure:

python test.py dev master-instace test-config-123.tgz
failed to download on 1.2.3.4
failed to download on 4.2.3.4
download action failed

In the case of verify action failure:

python test.py dev master-instace test-config-123.tgz
downloaded successfully on all 10 machines
failed to verify on 1.2.3.4
failed to verify on 4.2.3.4
verify action failed

or if there is any better way to make the output cleaner - I am open to the suggestion as well.

Answer 1

First of all, you did a pretty good job but there are definitely things that can be improved. Let's take them one by one.

1. Fix the bug

In Python, we can't do count++ so your code would immediately fail. There are some possible reasons for why this hasn't been adopted by Python core developers, some of them being:

• It generally encourages people to write less readable code;
• It mixes together statements and expressions, which is not good practice;
• Extra complexity in the language implementation, which is unnecessary in Python;

To fix this, just replace that statement with: count += 1. It does the exact same thing.

2. Remove unused imports and reorder them

It is always a good practice to cleanup your code after you've written it. If you would've done this, you would've noticed that there's an unused import: base64. Remove it.

Second, if you have some spare time, give PEP8 (the official Python styleguide) a read. There, you'll find out that the order of the imports can also be improved by grouping together stdlib imports (e.g.: json), 3rd party imports (e.g.: requests) and local specific imports. This has the advantage of making the code easier to read and the imports easier to spot. It might not seem too big of a deal now, but I've seen cases where there were like tens of imports used in the same file.

3. Digging into PEP8

I've already brought PEP8 into discussion and you should now be able to at least tell what it is. Let's try and apply some of its rules on your code:

3.1 Constants

Constants are usually defined on a module level and written in all capital letters with underscores separating words. Examples include MAX_OVERFLOW and TOTAL.

Exercise: figure out what constants you do have in your code snippet and do the proper changes

3.2 Naming conventions

• def update( url, json ): -> there should be no space before url and after json. You should also rename json into something else because it "Shadows name 'json' from outer scope" (this name is the same as the json library that you imported). A common convention here, if you still want to stick to this name, would be to add a _ after json. As a result, you'll have: def update(url, json):.

• print ("Http Error:",errh) -> there should be no space after print (or any other called function); you should (almost) always add a space after a comma. As a result, you'll have print("Http Error:", errh). There're other two things that I don't like here:

  a) The way you've formatted the string. There're other recommended ways of formatting a string: print("Http Error: {}".format(errh))or in Python3.6+ using [f-strings](https://docs.python.org/3/whatsnew/3.6.html#whatsnew36-pep498):print(f"Http Error: {errh}")`.

  b) What's errh? Wouldn't this be better named: http_error? I personally find naming variables / functions / classes / and so on to be the hardest thing in programming so I suggest you pay careful attention to this as better naming could save the future-you understand a piece of code you've written X years ago.

3.3 Comments

How is the following comment adding any value to your code: # define all constants? Try to ask you this question as if you're reading someone else's code and you'll do much better.

3.4 Maximum line length (79 chars)

This is really debatable but I just wanted you to be aware of it.

Some teams strongly prefer a longer line length. For code maintained exclusively or primarily by a team that can reach agreement on this issue, it is okay to increase the line length limit up to 99 characters, provided that comments and docstrings are still wrapped at 72 characters.

---

Let's look how your code would look like after applying all of the above:

import json
import sys

import requests


ACTIONS = ['download', 'verify']
ENDPOINT = "http://consul.{}.demo.com/"
CONFIG_PATH = "v1/kv/app-name/{}/config"
STATUS_PATH = "v1/kv/app-name/{}/status/{}"
CATALOG_PATH = "v1/catalog/service/app-name?ns=default&tag={}"
RAW = "?raw"


def update(url, json_):
    """
    This will make a PUT api call to update with new json config.
    """
    try:
        r = requests.put(url, data=json_, headers={'Content-type': 'application/json'})
        r.raise_for_status()
        return True
    except requests.exceptions.HTTPError as http_error:
        print(f"Http Error: {http_error}")
    except requests.exceptions.ConnectionError as connection_error:
        print(f"Error Connecting: {connection_error}")
    except requests.exceptions.Timeout as timeout_error:
        print(f"Timeout Error: {timeout_error}")
    except requests.exceptions.RequestException as request_error:
        print(f"Ops: Something Else: {request_error}")

    return False


def check(environment, instance, config, action):
    """
    This will get list of all ipAddresses for that instance in that environment.
    And then check whether each machine from that list have downloaded and 
    verified successfully. Basically for each 'action' type I need to verify things.
    If successful at the end then print out the message.
    """
    
    flag = True
    catalog_url = ENDPOINT.format(environment) + CATALOG_PATH.format(instance)
    response = requests.get(catalog_url)
    url = ENDPOINT.format(environment) + STATUS_PATH.format(instance) + RAW
    json_array = response.json()
    count = 0
    for x in json_array:
        ip = x['Address']
        count += 1
        r = requests.get(url.format(ip))
        data = r.json()
        if (
                action == 'download' 
                and "isDownloaded" in data 
                and data['isDownloaded'] 
                and "currentCfg" in data 
                and data['currentCfg'] == config
        ):
            continue
        elif (
                action == 'verify' and "activeCfg" in data 
                and data['activeCfg'] == config
        ):
            continue
        else:
            flag = False
            print(f"Failed to {action} on {ip}")

    if flag and action == 'download':
        print(f"Downloaded successfully on all {count} machines")
    elif flag and action == 'verify':
        print(f"Verified successfully on all {count} machines")

    return flag


def main():
    environment = sys.argv[1]
    instance = sys.argv[2]
    new_config = sys.argv[3]

    # make url for get api to verify whether configs are same or not
    # as mentioned in point 1.
    url = ENDPOINT.format(environment) + CONFIG_PATH.format(instance)
    response = requests.get(url + RAW)
    data = json.loads(response.content)
    remote_config = data['remoteConfig']
    
    if remote_config == new_config:
        print("cannot push same config")
        return

    # config is different
    data['remoteConfig'] = new_config
    
    # now for each action, update json and then verify whether that 
    # action completed successfully.
    for action in ACTIONS:
        data['action'] = action
        if update(url, json.dumps(data)):
            if check(environment, instance, new_config, action):
                continue
            else:
                print(f"{action} action failed")
                return
        else:
            print("failed to update")
            return


if __name__ == "__main__":
    try:
        sys.exit(main())
    except KeyboardInterrupt:
        print("\nCaught ctrl+c, exiting")

As you can see, there're not too many changes here but let's now see what we can do in regards to your code logic.

I'm going to do something a bit different. That means, I'm going to go through the problem description, not your code, and build this from scratch as I personally would.

Take few input parameters like - environmentName, instanceName, configName

For this, I'd build a function which parses the CLI arguments using argparse

def parse_cli_arguments():
    parser = argparse.ArgumentParser(description='My description here')
    parser.add_argument('--environment', required=True, help='Environment')
    parser.add_argument('--instance', required=True, help='Instance')
    parser.add_argument('--new_config', required=True, help='New config')
    return parser

---

Call get API on a service. Get the JSON data and compare the configName it already has vs what you want to update from the command line.

This has to be broken into multiple parts:

1. Call API endpoint and get the data:

class GetDataFailure(Exception):
    pass


def get_data(url, endpoint):
    try:
        response = requests.get(f'{url}{endpoint}')
        response.raise_for_status()
        return response
    except requests.exceptions.RequestException as request_error:
        raise GetDataFailure(str(request_error))

Above, I've created a custom exception which I'm raising if something happens with our request. You should also have in mind that's good practice to always set a default timeout when playing around with requests (exercise for you to figure out how).

2 & the rest: Now this is pretty confusing and I'm not sure your explanation of what should happen is clear enough. Let's say I don't have your code at my disposal and I want to follow your instructions. Do you think I'm gonna be able to do so given your steps?

I'd love to see how you'll manage to do from here so I recommend you do what I've told you so far, break your problem into specific & clear tasks and translate everything to code.

For example (similar to yours), let's say I want to fetch some data (JSON) from a URL and check if a specific value from that JSON matches my CLI argument. How would I start this?

1. What's the data I already have? A: URL, CLI argument, JSON value that I'm looking after. Write this down
2. Break everything into smaller steps:

• pase CLI arguments
• make a request to a specific url
• check that we have JSON data returned from that endpoint and return it as json
• check cli argument against JSON value (if any) and return True.
• add everything in main

Following the above, I'd suggest you rewrite your code and ask a separate question on code review.