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

Consider not always printing mutagen related information #13

Open
BuonOmo opened this issue Feb 22, 2022 · 4 comments · May be fixed by #14
Open

Consider not always printing mutagen related information #13

BuonOmo opened this issue Feb 22, 2022 · 4 comments · May be fixed by #14

Comments

@BuonOmo
Copy link

BuonOmo commented Feb 22, 2022

Hey,

First time contributing so first of all: thanks for the nice tool!

In my company we're users both of the original docker-compose for linux users and mutagen-compose for macos users. Unfortunately, the output is not the same, which makes it really bothering to script anything. For instance:

$ mutagen-compose ps --format json api
Forwarding sessions
--------------------------------------------------------------------------------
...
--------------------------------------------------------------------------------
Synchronization sessions
--------------------------------------------------------------------------------
...
--------------------------------------------------------------------------------
[{"ID":"...","Name":"...","Command":"...","Project":"stuart-api","Service":"api","State":"running","Health":"","ExitCode":0,"Publishers":[{"URL":"0.0.0.0","TargetPort":3000,"PublishedPort":3000,"Protocol":"tcp"}]}]

I'm really only expected the last line here. I've looked at the codebase, and there is not way to avoid this output (well > /dev/null or tail -1 would work, but I'd have to know beforehand that I need only one line, or none).

IMHO there should be a configuration available. Either ENV, which would be my favourite since it allows to set it beforehand and then work as if we were really using docker-compose, or some flag, which would be OK as well (we could define alias docker-compose='mutagen-compose --silence-mutagen' for a similar behaviour)

Anyway, I'd be glad to help on that one if that makes it ship faster 🚀
Otherwise, I'll wait :)

Cheers,
Ulysse

@xenoscopic
Copy link
Member

That's a fair point, for sure. The quick solution here might be to just disable any Mutagen session output if the --format flag is specified to ps. It's a little ugly to implement, but not too bad. I'm happy to do that if it would solve your problem.

Would that work for your use case?

Later down the road it might even be possible to include Mutagen session information as part of that JSON once Mutagen starts making JSONified session information available. If you're parsing the JSON, your parser can probably just ignore it.

@BuonOmo
Copy link
Author

BuonOmo commented Feb 22, 2022

That would only work to some extend. For instance, we use to mutagen-compose ps -q api which does not involve the --format flag, and I'm almost certain people out there may encounter the same issues with other situation. The point being: I think keeping both outputs as close as possible would make a lot of sense.

Hence I'd rather be in favour of some slower yet cleaner patch. Is there a reason why not? Sorry if my last message made it look like I'm in a hurry: I'm not, I just have some time now to help and may not have this amount of time later 🙂

The later down the road plan seems nice indeed, maybe following the x-mutagen convention?

@xenoscopic
Copy link
Member

Ah, okay, I see your point. Yeah, using --format probably isn't a sufficient heuristic.

I agree with your idea of using an environment variable. It's more consistent with Mutagen itself (which has a number of environment variables) and it's less tedious to use. It's also way easier to implement.

If you'd like to implement it, I'd be more than happy to merge it. You'd just need to wrap this code block in the appropriate check.

The biggest issue is just nomenclature. I'd say maybe call it MUTAGEN_COMPOSE_DISABLE_SESSION_LISTING and just implement the check as if os.Getenv("MUTAGEN_COMPOSE_DISABLE_SESSION_LISTING") != "1" to keep consistency with Mutagen's other environment variable checks (which don't do any sort of fancy environment value parsing - it's either "1" or ignored).

I can't think of any other locations in the output where Mutagen adds output that you'd want to disable, so I don't think the variable needs to be more generic or flexible than that. I guess one question would be whether or not you'd want to exclude the Mutagen sidecar container from being shown in the ps output, but I'd think not, and we could add a separate environment variable for that if desired. As far as showing Mutagen progress as part of the up command, I don't think anybody would want to disable that (nor is anybody parsing that), so hopefully this is the only location that needs adjustment.

I haven't had a chance to update this repository's contributing guidelines yet (I'll do it tomorrow), but you can follow the Mutagen guidelines for now. Nothing too onerous, but just try to keep the commit message consistent with this guidelines (e.g. ps: add environment variable to disable session listing), sign-off the commit to indicate agreement with the DCO, and try to structure the code to match what surrounds it.

@BuonOmo
Copy link
Author

BuonOmo commented Feb 23, 2022

Got it working as you expected (well you kind of gave most of the implementation away 😄)

$ MUTAGEN_COMPOSE_DISABLE_SESSION_LISTING=1 /path/to/build/mutagen-compose ps -q api
f16525cf1e1b9a32d59836af54b942fc236345fe74fba6080f52857d5ed41399

However, It looks like skipping the whole block would make it impossible to fail if there are errors related to mutagen. Hence I've provided a fixup! commit with a more local change. I'd be even more local if I could, but that would imply a PR to mutagen as well and maybe introducing some kind of logger to allow more granularity, hence I won't go down that path yet!

Here's the PR: #14

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 a pull request may close this issue.

2 participants