Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove uid override #58

Merged
merged 2 commits into from
Jan 16, 2025
Merged

Remove uid override #58

merged 2 commits into from
Jan 16, 2025

Conversation

alastairong1
Copy link
Contributor

This removes the logic that appends a global uid override to the app network seed. This is required as we move into production since the customer may already have a deployed network with a specific network seed that we must conform to. I.e. we cannot ask them to deploy with a new network seed that includes our uid.

Copy link
Contributor

@JettTech JettTech left a comment

Choose a reason for hiding this comment

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

@alastairong1 - This lib fn was being used as a utility for installing the core happs (in addition to the read only version of happs). The network seed reference was needed to differentiate between the same core apps that are in different nixos channels.

In the holo legacy versions, will we no longer require different nixos channels to have distinct networks for each of our core happs?

@alastairong1
Copy link
Contributor Author

@JettTech Thanks for that catch. We do need to be able to differentiate core-app networks so we don't cross-contaminate different legacy networks. However, customer apps do not have the same requirement - if we are asked to deploy the same customer app + network seed across two different environments we should be able to do that.

I'll look at changing to differentiate between core-app installations and customer app installations

@alastairong1
Copy link
Contributor Author

alastairong1 commented Jan 13, 2025

@JettTech Hmmm looking at the workflows again, could you double check that hpos-api-rust is used to install core-apps?

It looks like configure-holochain installs core-apps via its own connection to holochain rather than through hpos-api-rust?

https://github.com/Holo-Host/hpos-service-crates/blob/db6f7f675fb4997909bd43aef071551a52c6afdc/crates/configure-holochain/src/lib.rs#L76-L78

@JettTech
Copy link
Contributor

JettTech commented Jan 13, 2025

@alastairong1 - Thanks for double checking. You're right, it looks like we're still installing the the core-app dnas in the configure-holochain crate and relying on the special_installed_app_id to distinguish our core-app (hha + hf) from other apps within the holo-auto-installer. (There were some talks about consolidating/refactoring this repo into the service crates prior to the org change, so I was previously in that mindset.) So we do not need to change anything for the core-app (hha + hf) to accommodate this change.

The servicelogger instance for the installed core-app, however, is installed in the auto-installer loop and has been using the default of the DEV_UID_OVERRIDE as its network seed. I reviewed the code a bit more and don't see any immediate issues with installing the sl instance without without a network seed, as the injected sl dna props include vars which should be unique between channels (eg: bound_happ_id and holo_admin, etc), so I think this updated sl flow, will work cleanly across channels too. -- This said, I do think it's worthwhile keeping note of this sl instance installation change, as this has been a default for years and there may be tests or other tooling that expect this old setup.

Copy link
Contributor

@JettTech JettTech left a comment

Choose a reason for hiding this comment

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

Ready to ship. :shipit:

@alastairong1 alastairong1 merged commit 8206509 into develop Jan 16, 2025
1 of 2 checks passed
@alastairong1 alastairong1 deleted the remove-uid-override branch January 16, 2025 15:11
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.

2 participants