feat(publish): upload apps to App Hub #532

Birkbjo · 2021-03-11T16:17:18Z

This adds a command that is pretty similar to deploy. It extracts some information from the built appBundle, however this uploads that bundle to App Hub instead of a DHIS2-instance. There's some code that is duplicated from publish (eg. dumpHttpError), could probably extract some of this to a common file. I didn't want to change more code than necessary.

This should mainly be used with app-platform apps, and it's thus very simple to use with a bundled app. However I added an escape hatch so that apps that are not migrated to the platform may use this to upload any arbitrary bundle to the app hub. That's why we have options like appId, file and file-version. There are not intended to be use for overriding the config from a built app, as we do not want discrepancies from the uploaded app-information and the app-manifest.

How to test it?

API key generation is live on both staging and live server.

Log in to https://staging.apps.dhis2.org/
Navigate Api Key, and generate a new key
Use API-key with --apiKey or set as env with D2_apiKey=apiKey when using the CLI.

TODO:

Initial implementation
Initial docs

mediremi · 2021-03-22T10:29:52Z

cli/src/commands/publish.js

+ type: 'input',
+ name: 'minVersion',
+ message: 'Minimum DHIS2 version supported',
+ when: () => !params.minVersion,
+ validate: v =>
+ isValidServerVersion(v) ? true : 'Invalid server version',
+ },
+ {
+ type: 'input',
+ name: 'maxVersion',
+ message: 'Maximum DHIS2 version supported',
+ when: () => !params.maxVersion,
+ validate: v =>
+ !v || isValidServerVersion(v) ? true : 'Invalid server version',


Maybe the DHIS min and max versions should be specified in d2.config.js instead of as command parameters 🤔 It would make publishing through CI easier as the publish command and its parameters run by the CI action would not have to change

👍 agreed, I think that would be much cleaner. We can keep this as-is for now and add that later if we want (@Birkbjo would you prefer to do it now or in a separate change?)

I agree! That would be great, however I didn't want to make this dependent on adding support for that as well.

Add an issue to the CLI project in Jira so we don't forget?

cli/src/commands/publish.js

amcgee · 2021-04-09T08:55:42Z

@Birkbjo is this blocked by anything? Ready for a final review or not yet?

amcgee · 2021-04-09T09:09:05Z

Thought: maybe we should use the environment variable D2_APP_HUB_API_KEY or something a bit more explicit? D2_APIKEY could be for anything, we might have need of other api keys in the future?

Birkbjo · 2021-04-09T09:14:40Z

Thought: maybe we should use the environment variable D2_APP_HUB_API_KEY or something a bit more explicit? D2_APIKEY could be for anything, we might have need of other api keys in the future?

This is due to the built in resolution of env-vars to yargs params. Should I move away from using that in this case? Or else you would need to type --app_hub_api_key in params as far as I know.

amcgee · 2021-04-09T11:39:58Z

Yeah, I understand that, I don't think we should move away... I think it might be nice to have a more explicit version (in addition) since we might expect people to have the API key in their .bashprofile or something? Maybe `const resolvedApiKey = apiKey /* from cli */ || process.env.D2_APP_HUB_API_KEY || prompt()` ... ?

…

On Fri, Apr 9, 2021 at 11:15 AM Birk Johansson ***@***.***> wrote: Thought: maybe we should use the environment variable D2_APP_HUB_API_KEY or something a bit more explicit? D2_APIKEY could be for anything, we might have need of other api keys in the future? This is due to the built in resolution of env-vars to yargs params. Should I move away from using that in this case? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#532 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHHNMBGHDZSDGXIZ7KWPHDTH3ARLANCNFSM4ZASNGDQ> .

amcgee · 2021-05-07T08:35:37Z

cli/src/commands/publish.js

+}
+
+const promptForConfig = async params => {
+ if (process.env.CI && (!params.apikey || !params.minVersion)) {


@Birkbjo I think this will throw if the API key is passed as an env var in CI, right? We actually should prefer to use env vars in CI (since the api key is secret) so probably this should be:

const apiKey = params.apiKey || process.env.D2_APP_HUB_API_KEY if (process.env.CI && (!apiKey)) { ... } // ... when: () => !apiKey

Also - it looks like the option to specify minDHIS2Version in the config file was added, but this check happens before the config is parsed, is that incorrect?

@Birkbjo I think this will throw if the API key is passed as an env var in CI, right? We actually should prefer to use env vars in CI

Agree that we should prefer env-vars. But good catch, this would work with the previous env-var, as that would be resolved by yargs - but I totally forgot that this would not work when I changed the name of the env-vars.

Also - it looks like the option to specify minDHIS2Version in the config file was added, but this check happens before the config is parsed, is that incorrect?

At the moment it will only prompt for the version if used with the --file-parameter, if resolved from a d2-platform path, it will use the one in the manifest/d2.config. But yes you are correct, and I will remove that check as it's unnecessary.

amcgee

Looking good! I added some suggestions just to clean up the logic a touch

cli/src/commands/publish.js

amcgee · 2021-05-14T16:51:27Z

cli/src/commands/publish.js

+
+ await client.post(uploadAppUrl, formData, {
+ headers: formData.getHeaders(),
+ timeout: 30000, // Ensure we have enough time to upload a large zip file


Suggested change

timeout: 30000, // Ensure we have enough time to upload a large zip file

timeout: 300000, // Ensure we have enough time to upload a large zip file

For me on a mildy-fast internet connection I was sometimes having timeouts uploading an app, let's make this nice and long (5 minutes)

We should also ensure the app hub backend has a long timeout for that endpoint 🤔

@mediremi agreed, do you know what the timeout currently is? It's at least more than 30 seconds (the previous value) since I was able to upload after bumping the client timeout

As far as I can see we have not specified it anywhere, and the docs says its a default value of 10 seconds, but it's obviously not? Maybe it's different for streamed payloads.

The timeout on our load balancer in AWS is 60 seconds.

Since we support bundles of up to 20MB maybe we should try to increase that to ~5mins?

Sounds good to me. The docs on how to do so is here: https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/config-idle-timeout.html

We can do it using the AWS GUI as long as we don't destroy/recreate our environments that often.

We should document the AWS setup at some point as well so we know what customization we apply.

@varl that is different than timeout right? The timeout is for how long a request is allowed to take when uploading something. AWS describes that timeout as idle: " If no data has been sent or received by the time that the idle timeout period elapses, the load balancer closes the connection." Data is still being sent (hopefully) when uploading takes longer than that, so to me that doesn't seem to be the same thing as we're talking about here?

I'm not sure, you may be correct.

For each request that a client makes through a Classic Load Balancer, the load balancer maintains two connections. The front-end connection is between the client and the load balancer. The back-end connection is between the load balancer and a registered EC2 instance. The load balancer has a configured idle timeout period that applies to its connections.

This also stood out to me as relevant:

To ensure that lengthy operations such as file uploads have time to complete, send at least 1 byte of data before each idle timeout period elapses, and increase the length of the idle timeout period as needed.

The way I interpret the docs is that if a client uploads a file, it is sent to the load balancer, and when it's complete the load balancer sends it to the backend. It implies that the data isn't just streamed through one connection to another.

If that's true, then the docs makes sense to me since if it takes longer to upload the file to the load balancer than it maintains the connection to the backend, it would close the connection it has to the backend before the file upload completes.

I may be misunderstanding the docs, and I cannot find other docs explaining it clearly. We pay for the Load Balancer on a "per GB transferred" basis too, but I don't know if that means anything. 🤷

amcgee

Looks good, let's get this in! 🍾

# [6.2.0](v6.1.3...v6.2.0) (2021-05-28) ### Features * **publish:** upload apps to App Hub ([#532](#532)) ([b8c86b6](b8c86b6))

dhis2-bot · 2021-05-28T10:37:15Z

🎉 This PR is included in version 6.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Birkbjo requested a review from amcgee March 11, 2021 16:17

Birkbjo marked this pull request as draft March 11, 2021 16:58

Birkbjo changed the title ~~feat(publish): implement upload of version to App Hub~~ feat(publish): upload apps to App Hub Mar 11, 2021

Birkbjo marked this pull request as ready for review March 17, 2021 15:52

mediremi reviewed Mar 22, 2021

View reviewed changes

amcgee reviewed Mar 24, 2021

View reviewed changes

cli/src/commands/publish.js Show resolved Hide resolved

Birkbjo force-pushed the feat/publish-command branch from 6eb7659 to 87abbc0 Compare April 9, 2021 13:21

Birkbjo requested a review from amcgee April 16, 2021 12:27

Birkbjo added 10 commits May 3, 2021 13:31

feat(publish): implement upload of version to App Hub

0c777f4

fix: remove appId as required param

76cdb62

fix: update CI missing params error

0629ed5

refactor: cleanup

f48e8bd

fix: cleanup

9cea621

fix(refactor): rename apiKey to apikey

c801944

docs(publish): add docs for publish command

548a112

docs: update apikey option name

ab90c90

fix: validate config-file

3f2a10e

fix: use more specific env-var

e0df221

Birkbjo force-pushed the feat/publish-command branch from d6b3d81 to e0df221 Compare May 3, 2021 11:35

amcgee reviewed May 10, 2021

View reviewed changes

fix: remove check for minVersion - refactor reoslveBundle

f34f59a

amcgee reviewed May 14, 2021

View reviewed changes

Birkbjo added 2 commits May 19, 2021 20:26

refactor: fixes for review

e0a68cd

Merge branch 'master' into feat/publish-command

4577a7a

Birkbjo requested a review from amcgee May 21, 2021 11:58

mediremi approved these changes May 27, 2021

View reviewed changes

amcgee approved these changes May 27, 2021

View reviewed changes

Merge branch 'master' into feat/publish-command

ed7f5ab

Birkbjo merged commit b8c86b6 into master May 28, 2021

Birkbjo deleted the feat/publish-command branch May 28, 2021 10:31

dhis2-bot added a commit that referenced this pull request May 28, 2021

chore(release): cut 6.2.0 [skip ci]

4f939c3

# [6.2.0](v6.1.3...v6.2.0) (2021-05-28) ### Features * **publish:** upload apps to App Hub ([#532](#532)) ([b8c86b6](b8c86b6))

dhis2-bot added the released label May 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(publish): upload apps to App Hub #532

feat(publish): upload apps to App Hub #532

Birkbjo commented Mar 11, 2021 •

edited

Loading

mediremi Mar 22, 2021

amcgee Mar 24, 2021

Birkbjo Mar 24, 2021

varl May 25, 2021

amcgee commented Apr 9, 2021 •

edited

Loading

amcgee commented Apr 9, 2021 •

edited

Loading

Birkbjo commented Apr 9, 2021 •

edited

Loading

amcgee commented Apr 9, 2021 via email

amcgee May 7, 2021

Birkbjo May 10, 2021 •

edited

Loading

Birkbjo May 10, 2021 •

edited

Loading

amcgee left a comment

amcgee May 14, 2021

mediremi May 14, 2021

amcgee May 19, 2021

Birkbjo May 19, 2021

varl May 25, 2021

mediremi May 25, 2021

varl May 26, 2021

Birkbjo May 27, 2021 •

edited

Loading

varl May 27, 2021

amcgee left a comment

dhis2-bot commented May 28, 2021

	timeout: 30000, // Ensure we have enough time to upload a large zip file
	timeout: 300000, // Ensure we have enough time to upload a large zip file

feat(publish): upload apps to App Hub #532

feat(publish): upload apps to App Hub #532

Conversation

Birkbjo commented Mar 11, 2021 • edited Loading

How to test it?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcgee commented Apr 9, 2021 • edited Loading

amcgee commented Apr 9, 2021 • edited Loading

Birkbjo commented Apr 9, 2021 • edited Loading

amcgee commented Apr 9, 2021 via email

Choose a reason for hiding this comment

Birkbjo May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Birkbjo May 10, 2021 • edited Loading

Choose a reason for hiding this comment

amcgee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Birkbjo May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcgee left a comment

Choose a reason for hiding this comment

dhis2-bot commented May 28, 2021

Birkbjo commented Mar 11, 2021 •

edited

Loading

amcgee commented Apr 9, 2021 •

edited

Loading

amcgee commented Apr 9, 2021 •

edited

Loading

Birkbjo commented Apr 9, 2021 •

edited

Loading

Birkbjo May 10, 2021 •

edited

Loading

Birkbjo May 10, 2021 •

edited

Loading

Birkbjo May 27, 2021 •

edited

Loading