-
Notifications
You must be signed in to change notification settings - Fork 151
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
feat: Make .meltano
folder path configurable via new MELTANO_SYS_DIR_ROOT
env var
#6628
Conversation
👷 Deploy request for meltano pending review.Visit the deploys page to approve it
|
@rawwar - Thanks for this contribution! For my part, the implementation looks pretty clean. I'll let @pandemicsyn take the official review though. I've approved CI tests to run on this PR. Feel free to move out of "Draft" status when you see tests passing. @pandemicsyn, @edgarrmondragon, @tayloramurphy - The only things I see to pause on here is the naming of the environment variable - and whether this needs to be a setting or if a env var parse is fine. Some options regarding possible names/spellings:
I think I like following the convention of all env vars starting with |
@aaronsteers since we have |
I'm a no-go on Beyond that I have no prefs on naming. Sounds like MELTANO_SYS_DIR_ROOT is the defacto choice then ? |
The only reason I raised it as a Draft PR was that I KNEW that variable name was bad 😄 . I'll use |
My vote would be to consider $MELTANO_SYS_DIR_ROOT as a replacement to .meltano. So that the default is implied to be the equivalent of:
but you could override it too:
@aaronsteers @tayloramurphy curious though if y'all where thinking about it the same way. |
Co-authored-by: Edgar R. M. <edgarrm358@gmail.com>
👍👍 |
I'm a bit stuck on how to correct
If |
@rawwar with the changes I suggested, the default dot meltano dir would live in |
But, From the above discussion, we want to consider |
@rawwar @edgarrmondragon yea wouldn't the default entry just become:
Which by default then resolves $MELTANO_SYS_DIR_ROOT to $MELTANO_PROJECT_ROOT/.meltano ? |
Correct me If I am wrong.
when user sets
|
@rawwar Yes, you're right! I think that's the correct approach. |
I just realized while trying to write tests for #6627 that I need to write tests for this as well. I might take a while for writing tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggested change, but otherwise approving from a docs perspective.
Co-authored-by: Taylor A. Murphy <taylor@meltano.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ 👍
@rawwar I think it makes sense for the |
I have been trying to implement this. Was having trouble doing it properly. Edit1: Giving more info |
As of now, I don't think we have a formal dev standard for this. But the above is consistent with what we might adopt as a standard if we take this as reference: From https://clig.dev/#configuration:
cc @pandemicsyn, who is taking lead on CLI dev standards: |
+1 we've not codified anything as far a control layers but pretty sure when it comes to a hierarchy what @aaronsteers referenced will almost certainly be our default stance.
@rawwar did you manage to work around this, or is this still a sticking point for ya? |
I did try a week ago. But, I could not. I was stuck at it. I'll be looking into it today. I'll give an update here once I'm done. |
Hey @rawwar, do you need need help to push this over the finish line? |
I had some personal things going on. I will try to look into it this weekend. If I can't complete it, I do need help. Thank you so much! |
I tried to resolve the issue since weekend and I am unable to. Would appreciate if I can get some help with this |
@rawwar I'll take a look |
d4e1db1
to
feda7e9
Compare
feda7e9
to
ea199f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rawwar ok, I got the merge conflicts solved after a few attempts 😅.
This is looking good. Pinging @aaronsteers in case I miss anything.
.meltano
folder path configurable.meltano
folder path configurable via new MELTANO_SYS_DIR_ROOT
env var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edgarrmondragon - Looks good from my side. Just one question here: https://github.com/meltano/meltano/pull/6628/files#r1000023143 [Resolved!]
If all other issues/questions resolved, do you want to do the honors with a merge to main
?
@edgarrmondragon .. Thank you so much! |
draft PR for #6619