NationTech/harmony
6
0
Fork 1
Code Issues 32 Pull Requests 7 Actions 7 Packages Projects Releases 1 Wiki Activity

fix: unjank the demo #85

Merged
taha merged 19 commits from fix_demo into master 2025-07-11 14:32:22 +00:00
Conversation 0 Commits 19 Files Changed 20 +299 -158
taha commented 2025-07-09 04:39:20 +00:00
Collaborator
Copy Link
No description provided.
taha added 10 commits 2025-07-09 04:39:23 +00:00
push using bollard
Some checks failed
Run Check Script / check (pull_request) Failing after -44s
Details
28476af222
taha changed title from fix: unjank the demo to WIP: fix: unjank the demo 2025-07-09 04:39:36 +00:00
johnride requested changes 2025-07-09 08:52:38 +00:00
johnride left a comment
Owner
Copy Link

Overall improvement, going in the right direction.

I'm not convinced about bollard as it adds another layer of abstraction, dependency and potential bugs for what seems to me little improvement in readability and capability. But I see some interesting things so it's fine for now.

Make sure to respect the mindset of a Harmony application to provide opinionated infrastructure features for modern applications as easily as possible for a developer that doesn't want anything to do with it.

What is important to them is to receive alerts when they have something going wrong. We must be able to use whatever alerting stack implementation under the hood as we want.

The way it is written in this p-r, we're stuck forever with ntfy and prometheus (or at least, this application is).

Always keep in mind that this application definition must work today, but also next year and 5 years from now when we have moved away from all these tools to adopt the new generation of alerting or whatever, without any change.

The idea is that the Application score is a "write once, run forever" thing.

Overall improvement, going in the right direction. I'm not convinced about bollard as it adds another layer of abstraction, dependency and potential bugs for what seems to me little improvement in readability and capability. But I see some interesting things so it's fine for now. Make sure to respect the mindset of a Harmony application to provide opinionated infrastructure features for modern applications as easily as possible for a developer that doesn't want anything to do with it. What is important to them is to receive alerts when they have something going wrong. We must be able to use whatever alerting stack implementation under the hood as we want. The way it is written in this p-r, we're stuck forever with ntfy and prometheus (or at least, this application is). Always keep in mind that this application definition must work today, but also next year and 5 years from now when we have moved away from all these tools to adopt the new generation of alerting or whatever, without any change. The idea is that the Application score is a "write once, run forever" thing.
.gitignore
@ -2,3 +2,7 @@ target
private_repos
log/
*.tgz
examples/rust/examples/rust/webapp/helm/
johnride commented 2025-07-09 08:28:23 +00:00
Owner
Copy Link

These would be better in the examples's own gitignore file examples/rust/.gitignore. Eventually we may have hundreds of examples, we don't want thousands of lines in this file.

But thinking a bit further, this points towards a problem on the way we manage these generated files. They should probably end up in a harmony specific directory that we can then ignore in a single line for all examples. Maybe harmony_generated or harmony_build ?

These would be better in the examples's own gitignore file `examples/rust/.gitignore`. Eventually we may have hundreds of examples, we don't want thousands of lines in this file. But thinking a bit further, this points towards a problem on the way we manage these generated files. They should probably end up in a harmony specific directory that we can then ignore in a single line for all examples. Maybe `harmony_generated` or `harmony_build` ?
examples/rust/src/main.rs Outdated
@ -14,2 +28,4 @@
async fn main() {
env_logger::init();
let tenant = TenantScore {
johnride commented 2025-07-09 08:32:40 +00:00
Owner
Copy Link

The Tenant should not be visible at the applcation scope. An application will auto detect the tenant it is supposed to be in from the environment (such as the content of the loaded kubeconfig).

For example, a client cannot provision its own tenant as it may not have permission on the host cluster to do so.

There may be exceptions and more complex use cases coming up soon, but for the sake of this example we want to keep it 100% focused on the application and especially how simple it it to deploy a fully featured infrastructure around an application with as close to zero config as possible.

The Tenant should not be visible at the applcation scope. An application will auto detect the tenant it is supposed to be in from the environment (such as the content of the loaded kubeconfig). For example, a client cannot provision its own tenant as it may not have permission on the host cluster to do so. There may be exceptions and more complex use cases coming up soon, but for the sake of this example we want to keep it 100% focused on the application and especially how **simple** it it to deploy a fully featured infrastructure around an application with as close to zero config as possible.
examples/rust/src/main.rs Outdated
@ -20,21 +61,59 @@ async fn main() {
framework: Some(RustWebFramework::Leptos),
});
let ntfy = NtfyScore {
johnride commented 2025-07-09 08:34:03 +00:00
Owner
Copy Link

Our users have no clue what ntfy or prometheus is, this is leaking Harmony's internals. Alerting is just a feature of the infrastructure that they do not need to know how it is implemented.

Our users have no clue what ntfy or prometheus is, this is leaking Harmony's internals. Alerting is just a feature of the infrastructure that they do not need to know how it is implemented.
harmony/src/modules/application/features/helm_argocd_score.rs Outdated
@ -58,2 +58,4 @@
.await?;
let gvk = GroupVersionKind::gvk("argoproj.io", "v1alpha1", "Application");
let api_resource = ApiResource::from_gvk_with_plural(&gvk, "applications");
johnride commented 2025-07-09 08:38:23 +00:00
Owner
Copy Link

This is binding Harmony's usage to kube-rs API, which is wrong.

I think it would be doable to extract the gvk from the manifest itself and have apply_yaml return Err("Could not build gvk from input yaml, missing property {missing_property}") . Then I think the only missing bit will be the plural which is not kube-rs specific so more acceptable.

This is binding Harmony's usage to kube-rs API, which is wrong. I think it would be doable to extract the gvk from the manifest itself and have `apply_yaml` return `Err("Could not build gvk from input yaml, missing property {missing_property}")` . Then I think the only missing bit will be the plural which is not kube-rs specific so more acceptable.
harmony/src/modules/application/rust.rs Outdated
@ -438,3 +449,3 @@
name: {{ include "chart.fullname" . }}
spec:
type: {{ .Values.service.type }}
type: {{ $.Values.service.type }}
johnride commented 2025-07-09 08:46:29 +00:00
Owner
Copy Link

What does the $ do ?

What does the `$` do ?
taha commented 2025-07-09 14:17:19 +00:00
Author
Collaborator
Copy Link

$ is the global scope, for some reason .Values.service.type on its own wasn't working (different scope?), did some digging, found $ to specifically reference the global/top level scope and it then worked. I'm not an expert at helm templating though

`$` is the global scope, for some reason `.Values.service.type` on its own wasn't working (different scope?), did some digging, found `$` to specifically reference the global/top level scope and it then worked. I'm not an expert at helm templating though
harmony/src/modules/application/rust.rs
@ -443,1 +453,3 @@
targetPort: 3000
- name: main
port: {{ $.Values.service.port | default 3000 }}
targetPort: {{ $.Values.service.port | default 3000 }}
johnride commented 2025-07-09 08:46:11 +00:00
Owner
Copy Link

Thanks for fixing my demo hardcoding crap 😅

Thanks for fixing my demo hardcoding crap 😅
taha added 2 commits 2025-07-10 13:51:22 +00:00
Move ntfy into monitoring feature
Some checks failed
Run Check Script / check (pull_request) Failing after 2m5s
Details
d7de1ea752
taha added 1 commit 2025-07-10 15:25:06 +00:00
forgot to push
Some checks failed
Run Check Script / check (pull_request) Failing after 1m22s
Details
cc4529617c
taha added 1 commit 2025-07-10 15:28:00 +00:00
remove tenant score
Some checks failed
Run Check Script / check (pull_request) Failing after 1m3s
Details
81bee7e12a
wjro reviewed 2025-07-10 15:41:10 +00:00
harmony/src/modules/application/features/argo_types.rs Outdated
@ -91,3 +88,3 @@
project: Default::default(),
source: Source {
repo_url: Url::parse("http://asdf").expect("Couldn't parse to URL"),
repo_url: "http://asdf".to_string(),
wjro commented 2025-07-10 15:41:10 +00:00
Owner
Copy Link

I would write some documentation for why you replaced url with string

I would write some documentation for why you replaced url with string
wjro reviewed 2025-07-10 15:42:28 +00:00
harmony/src/modules/application/rust.rs
@ -5,2 +5,4 @@
use async_trait::async_trait;
use bollard::query_parameters::PushImageOptionsBuilder;
use bollard::{Docker, body_full};
wjro commented 2025-07-10 15:42:28 +00:00
Owner
Copy Link

add a comment to justify using bollard vs docker via the cli

add a comment to justify using bollard vs docker via the cli
taha commented 2025-07-10 17:28:12 +00:00
Author
Collaborator
Copy Link

The reason I used bollard was because I find it preferable to access the docker daemon programmatically over an API over using a (albeit relatively stable) CLI interface

The reason I used `bollard` was because I find it preferable to access the docker daemon programmatically over an API over using a (albeit relatively stable) CLI interface
wjro marked this conversation as resolved
wjro requested changes 2025-07-10 15:43:39 +00:00
wjro left a comment
Owner
Copy Link

Just needs some documentation to justify some choices but overall looks good to me

Just needs some documentation to justify some choices but overall looks good to me
taha added 1 commit 2025-07-10 17:26:31 +00:00
worked with will to get monitoring + ntfy demoed/tested
Some checks failed
Run Check Script / check (pull_request) Failing after -47s
Details
d0d80aee28
taha added 1 commit 2025-07-10 17:30:29 +00:00
cargo fmt/fix
Some checks failed
Run Check Script / check (pull_request) Failing after -48s
Details
c573e7f64c
wjro approved these changes 2025-07-10 18:14:14 +00:00
harmony/src/modules/application/features/monitoring.rs Outdated
@ -19,4 +21,4 @@
#[derive(Debug, Default, Clone)]
pub struct Monitoring {}
wjro commented 2025-07-10 18:12:25 +00:00
Owner
Copy Link

if we add pub application: Arc to monitoring we can use the application name as namespace so that it is consistent with the way continuous _delivery feature gets the namespace

if we add pub application: Arc<dyn Application> to monitoring we can use the application name as namespace so that it is consistent with the way continuous _delivery feature gets the namespace
taha marked this conversation as resolved
harmony/src/modules/application/features/monitoring.rs
@ -27,0 +36,4 @@
// .expect("couldn't get tenant config")
// .name,
namespace: "harmonydemo-staging".to_string(),
host: "localhost".to_string(),
wjro commented 2025-07-10 18:12:36 +00:00
Owner
Copy Link

rather than hardcoding namespace here

rather than hardcoding namespace here
taha marked this conversation as resolved
taha changed title from WIP: fix: unjank the demo to fix: unjank the demo 2025-07-11 13:41:51 +00:00
taha added 1 commit 2025-07-11 13:41:55 +00:00
fix app name/namespace
Some checks failed
Run Check Script / check (pull_request) Failing after 1m11s
Details
694f0dc045
taha added 1 commit 2025-07-11 14:12:48 +00:00
fix example
Some checks failed
Run Check Script / check (pull_request) Failing after 2m53s
Details
b97c58bc34
taha added 1 commit 2025-07-11 14:23:42 +00:00
update test
All checks were successful
Run Check Script / check (pull_request) Successful in 2m56s
Details
17ebdc1247
taha merged commit 3be2fa246c into master 2025-07-11 14:32:22 +00:00
taha deleted branch fix_demo 2025-07-11 14:32:24 +00:00
taha referenced this issue from a commit 2025-07-11 14:32:26 +00:00
fix: unjank the demo (#85)
Sign in to join this conversation.
Reviewers
No reviewers
johnride
wjro
Labels
Clear labels   
Architecture Decision Record

   
Good first issue

   
RFC
No Label
Architecture Decision Record
Good first issue
RFC
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: NationTech/harmony#85
No description provided.
Delete Branch "fix_demo"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?