PLAT-200 jsr config importing #44
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To make a generic config importer PLAT-200
To add the await-helper docker-compose file so that the service can be verified, and to update the logic in swarm.sh to run the config importer PLAT-200
To launch the config importer, the compose file is added. And a base version of the export.jsrexport file has been added, so that the setup_repo go function will know what to replace PLAT-200
To stick to the conventions laid out by team badger PLAT-200
To keep stale containers and configs from remaining. PLAT-200
To reference the same library and avoid rewriting the same timeout check code. The necessary calling of functions with function argument is also implemented in this commit. PLAT-200
To provide better detail on using the library PLAT-200
The config importer isn't a platform package, so it should be in it's own repo PLAT-200
To match the variables called for the config importer PLAT-200
PLAT-200
CastelloG
suggested changes
Apr 20, 2022
dashboard-visualiser-jsreport/importer/docker-compose.config.yml
Outdated
Show resolved
Hide resolved
To make it more specific to the JSR use case, the name is now JS_REPORT_SSL PLAT-200
There was code that was used for testing only, and it was forgotten in the code base, this commit simply removes it. PLAT-200
To use config digests PLAT-200
To environment variables to use the BODY_DATA_FORMAT instead of making the config importer specific to a particular service PLAT-200
The default value was accidently changed when renaming the variable PLAT-200
CastelloG
reviewed
Apr 21, 2022
1) In case the user forgets to add the time parameters 2) These are standard across a few packages in platform, this way the user can specify custom times if only needed PLAT-200
CastelloG
reviewed
Apr 21, 2022
CastelloG
reviewed
Apr 21, 2022
The default times were forgotten as times used for testing PLAT-200
PLAT-200
There are default values for the timeout, and the intent is for the values to remain default PLAT-200
CastelloG
reviewed
Apr 22, 2022
Function arguments were specified in the wrong order PLAT-200
To include only the environment variables that are being set PLAT-200
Mark-Labuschagne
changed the title
To use default values for describing env vars PLAT-200
rcrichton
reviewed
Apr 25, 2022
| fi | ||
| done | ||
|
|
||
| docker service rm instant_jsreport-config-importer |
There was a problem hiding this comment.
Docker swarm has the concept of jobs. I know I'm mentioned this before, but maybe we should start using them. Then we might not need to remove the service once it's done as it gets marked as completed and it's easier to then see what has executed against the stack. Check the docs here - https://docs.docker.com/engine/reference/commandline/service_create/#running-as-a-job
There was a problem hiding this comment.
Okay, so I've given this a go. There are a few reasons I don't really like it.
- We need to move our raft configs from the docker-compose.config.yml files to our base docker-compose for that service - this leads to a lack of separation-of-concerns
- We need to either specify a lot of env vars in the swarm.sh where we create the job, or we need to specify even more env vars in our .env files (which are already getting quite cluttered), whereas the compose.config.yml was a nice place to list the env vars needed for the api-config-importer to run.
- Reading env vars are proving to be a hassle (I've given it at least 35 minutes now and I'm still getting problems)
Overall, if it were my choice, I would keep it the way we have it. I think it's simpler and a bit cleaner, too.
There was a problem hiding this comment.
Hmm, ok cool. I thought you would be able to specify the mode in the docker compose which would make the change fairly simple. However it seems that isn't possible yet - docker/cli#2907
Seems job is only supported via the CLI, which is annoying.
CastelloG
reviewed
Apr 26, 2022
| SERVICE_API_PORT: 5488 | ||
| API_USERNAME: ${JS_REPORT_USERNAME:-admin} | ||
| API_PASSWORD: ${JS_REPORT:-dev_password_only} | ||
| JS_REPORT_SSL: ${JS_REPORT_SSL:-true} |
There was a problem hiding this comment.
Shouldnt this be:
Suggested change
| JS_REPORT_SSL: ${JS_REPORT_SSL:-true} | |
| SSL : ${JS_REPORT_SSL:-true} |
There was a problem hiding this comment.
Yes, it definitely should be. Done in 9512b9a
PLAT-200
To import the name of the config file through an env var. This has to be implemented to match the code now in the api-config-importer PLAT-200
Updated values for JSR resource allocations, updated package-metadata to match and also added variables that should have been there. Also added a default value for config_file PLAT-200
tumbledwyer
reviewed
Apr 26, 2022
tumbledwyer
reviewed
Apr 26, 2022
tumbledwyer
suggested changes
Apr 26, 2022
In the dependencies section, ElasticSearch is referenced, and the package ID was spelled incorrectly PLAT-200
tumbledwyer
approved these changes
Apr 28, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.