Skip to content

Configure aklite with compose apps#17

Merged
mike-sul merged 1 commit intomasterfrom
FF-160/conf-lite-for-compose-apps
Jun 2, 2020
Merged

Configure aklite with compose apps#17
mike-sul merged 1 commit intomasterfrom
FF-160/conf-lite-for-compose-apps

Conversation

@mike-sul
Copy link
Copy Markdown
Contributor

Signed-off-by: Mike Sul [email protected]

@mike-sul mike-sul requested a review from doanac May 27, 2020 16:43
@doanac
Copy link
Copy Markdown
Member

doanac commented May 27, 2020

that was easier than I expected! LGTM

@mike-sul
Copy link
Copy Markdown
Contributor Author

Verified on QEMU/i7 against foundriesio/aktualizr-lite#9, it worked for me.

Comment thread src/main.cpp Outdated
device.put("overrides.pacman.docker_apps_root", "\"" + apps_root + "\"");
if (!options.docker_apps.empty()) {
device.put("overrides.pacman.docker_apps", "\"" + options.docker_apps + "\"");
string apps_root = options.sota_config_dir + "/compose_apps";
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@doanac I wonder if we should call this dir compose_apps as well as the command line param as compose-apps, maybe leave it as docker* since docker term is more common (I am subjective here) or maybe docker-compose-apps?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been leaving "docker" out because:

  • docker-compose-apps is a mouthful
  • docker-apps and docker-compose-apps look really similar.

I almost wonder if we should call this cli argument "apps". Then the parameter is good either way.

Another thing I realized we need to check for: We should only enable this new logic if "DOCKER_COMPOSE_APP" is defined in the factory-config.yml.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I almost wonder if we should call this cli argument "apps".

+1, What about the configuration variables in sota.toml and the apps directory name?

Copy link
Copy Markdown
Contributor Author

@mike-sul mike-sul May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only enable this new logic if "DOCKER_COMPOSE_APP" is defined in the factory-config.yml.

Do you mean taking into account it in the lmp-device-register recipe? I suppose a package manager type should be defined anyway. This is actually not a new logic just renaming.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@doanac let's chat and make a decision on this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean taking into account it in the lmp-device-register recipe?

Correct. We'd need to set this in our lmp build logic around here:

https://github.com/foundriesio/ci-scripts/blob/master/lmp/bb-config.sh#L27

and then the recipe could key off that value if it exists.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the configuration variables in sota.toml and the apps directory name?

I don't have a strong opinion on the variable names. As per the "apps" directory. I noticed that on my local test device I had simply named the directory /var/sota/compose. I kind of like just calling it "compose" - once its on disk its just vanilla compose. Its only the transports that's doing something new.

@mike-sul
Copy link
Copy Markdown
Contributor Author

mike-sul commented Jun 1, 2020

@doanac Renamed compose-apps to apps

@mike-sul mike-sul force-pushed the FF-160/conf-lite-for-compose-apps branch from 3b2c4fd to bc452db Compare June 1, 2020 17:39
@doanac
Copy link
Copy Markdown
Member

doanac commented Jun 1, 2020

looks good. I think we should get @ricardosalveti to take a look as well.

@mike-sul
Copy link
Copy Markdown
Contributor Author

mike-sul commented Jun 2, 2020

@ricardosalveti Please, review it

Copy link
Copy Markdown
Member

@ricardosalveti ricardosalveti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks clean.

@mike-sul mike-sul merged commit c1c5781 into master Jun 2, 2020
@mike-sul mike-sul deleted the FF-160/conf-lite-for-compose-apps branch June 2, 2020 15:57
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.

3 participants