Skip to content
This repository has been archived by the owner on May 6, 2020. It is now read-only.

Adding procfile_structure on app api #1321

Closed
wants to merge 9 commits into from
Closed

Adding procfile_structure on app api #1321

wants to merge 9 commits into from

Conversation

Cryptophobia
Copy link
Contributor

@Cryptophobia Cryptophobia commented Aug 15, 2017

The /app route should expose the procfile_structure of the app release. Since the structure on the /app route is not indicative of the actual deployable processes of the application, there should be another json structure that can tell us what the procfile is actually set to for the latest release. When using procfile_structure we can tell which of the processes exist in our procfile at this particular release:

The idea is that we can use procfile_structure for scaling and validating which processes we can actually scale through the /scale route. This is so because a call to .../app/{app_name}/scale with a payload {"worker": 1} would actually fail with 404 Not Found and this return from the api:

{
    "detail": "Container type worker does not exist in application"
}

Our app structure is below:

    "uuid": "db58126d-a76b-428c-9741-1b8d09bed2b7",
    "id": "testing-application-procfile",
    "owner": "admin",
    "structure": {
        "worker": 0,
        "newone": 0,
        "scheduler": 0,
        "web": 1,
        "testing": 0
    },
    "procfile_structure": {
        "web": "node server.js",
        "test": "node server.js"
    },
    "created": "2017-08-15T15:32:27Z",
    "updated": "2017-08-15T17:12:12Z"
}

But a call for each of the processes in procfile_structure would not fail the scale call.

This has been tested extensively with git remote application deployments and Docker image pull deployments.

The tests are not written yet.
Code in deis-cli has not been written yet.

@deis-admin
Copy link

Thanks for the contribution! Please ensure your commits follow our style guide. This code will be tested once a Deis maintainer reviews it.

@mboersma
Copy link
Member

Jenkins, add to whitelist.

@Cryptophobia
Copy link
Contributor Author

We implemented procfile_structure because structure was for a different intended behaviour and DEIS_DEPLOY_PROCFILE_MISSING_REMOVE variable did not solve our problem as you can see per issue here: #1320

@Cryptophobia
Copy link
Contributor Author

@mboersma Can you rerun Jenkins? I reset the top commit and fixed the whitespacing issues.

@mboersma
Copy link
Member

Jenkins, test this please.

@Cryptophobia
Copy link
Contributor Author

Jenkins, look me in the eye and tell me that's not a 4 space indent!

Jenkins has been hanging out with the wrong group of linting styling friends.

@bacongobbler
Copy link
Member

I think that's just a flake8 oddity. Here's another example of a multi-line if statement in the codebase. Basically you need to indent it to 4 spaces relative to the if statement. Flake8 is detecting the indentation level at 7 spaces rather than the typical 4.

@Cryptophobia
Copy link
Contributor Author

Cryptophobia commented Aug 16, 2017

@bacongobbler Thanks! Indeed flake8 seems to have a different idea of where the indent should start at, it was based on where the if statement starts.

Ran flake8 locally and pushed with the 4 space indents now.

@mboersma
Copy link
Member

One of the unit tests is still failing:

09:26:13 ======================================================================
09:26:13 FAIL: test_response_data (api.tests.test_app.AppTest)
09:26:13 Test that the serialized response contains only relevant data.
09:26:13 ----------------------------------------------------------------------
09:26:13 Traceback (most recent call last):
09:26:13   File "/usr/local/lib/python3.5/dist-packages/requests_mock/mocker.py", line 222, in inner
09:26:13     return func(*args, **kwargs)
09:26:13   File "/usr/lib/python3.5/unittest/mock.py", line 1157, in patched
09:26:13     return func(*args, **keywargs)
09:26:13   File "/test/rootfs/api/tests/test_app.py", line 93, in test_response_data
09:26:13     self.assertIn(key, ['uuid', 'created', 'updated', 'id', 'owner', 'structure'])
09:26:13 AssertionError: 'procfile_structure' not found in ['uuid', 'created', 'updated', 'id', 'owner', 'structure']

@Cryptophobia
Copy link
Contributor Author

Cryptophobia commented Aug 17, 2017

@mboersma Thanks for checking! Fixed now!

I should run the docker testing suite locally before pushing. I guess "making it rain" in Jenkins is a bad thing... 🌧

@Cryptophobia
Copy link
Contributor Author

Cryptophobia commented Aug 17, 2017

@mboersma : Any idea on this failure, I did not get this locally with make test command. Its failing because of the kube cluster tags?

@mboersma
Copy link
Member

@Cryptophobia looks like an intermittent failure. :-( It passed on a second run.

@mboersma mboersma added this to the v2.18 milestone Aug 17, 2017
@Cryptophobia
Copy link
Contributor Author

Thanks! Can we agree since this happens 50% of the time, it is a heisenbug 38% of the time? 😸

@mboersma
Copy link
Member

mboersma commented Sep 7, 2017

I can see the usefulness of this, but I fear we haven't had time to review it properly yet and the v2.18 release time is upon us. @Cryptophobia would it be ok to leave this PR open for future efforts? Alternatively, we could slip the release a day or two in order to get proper review and testing done.

@mboersma mboersma removed this from the v2.18 milestone Sep 7, 2017
@Cryptophobia
Copy link
Contributor Author

@mboersma , sounds like it did not go into this release. when would there be time to review this proprerly, is that even in the roadmap after September?

@bacongobbler
Copy link
Member

This was the final feature release, so this won't be able to land any time after September.

To clarify on the timeline from the EOL blog post:

  • 08/08/2017 Deis Workflow v2.17
  • 09/07/2017 Deis Workflow v2.18 final release before entering maintenance mode
  • 03/01/2018 End of Workflow maintenance: critical patches no longer merged

"Maintenance mode" means no more features. Only critical fixes or security releases will go into Workflow at this point.

@Cryptophobia
Copy link
Contributor Author

Cryptophobia commented Sep 21, 2017

This PR has been recreated for the https://github.com/deisthree fork here: teamhephy/controller#2

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

Successfully merging this pull request may close these issues.

4 participants