fix: Ignore default environment for some commands #6862
Conversation
✅ Deploy Preview for meltano ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Codecov Report
@@ Coverage Diff @@
## main #6862 +/- ##
==========================================
+ Coverage 88.68% 88.71% +0.02%
==========================================
Files 283 283
Lines 20405 20419 +14
Branches 2010 2011 +1
==========================================
+ Hits 18097 18114 +17
+ Misses 1972 1970 -2
+ Partials 336 335 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@WillDaSilva love this PR coming through. We'll need to update the docs in the CLI reference section to make it explicit about which ones require an environment or not. Maybe outside the scope of this PR, but I'm wondering if some short of small table at the start would be useful where we list attributes of the command such as "Uses At a minimum though we should update the text of every command to have a one-liner on whether or not @aaronsteers what else would you like to see here? |
|
@WillDaSilva - This looks really great and I like the design choices you made here with the two functions handling the two use cases. @tayloramurphy - I just spent some time in the docs, and writing suitable copy doesn't seem like a trivial task. In many commands, an immediate mentioning of environment-based usage does not look like it would work with the exisitng context. I did find an example under invoke which could be adapted:
But that phrasing is not ideal. What about these proposals, coming after each command's "How to Use" subsection: Case 1 (Environment always ignored):
Case 2 (Get only from
Case 3 (Get from
Case 4: (Environment always required)
@tayloramurphy - Feel free to make any changes you'd like to see. But to minimize the number of iterations here in the code itself, would be good to have copy that @WillDaSilva can insert directly. @WillDaSilva - I don't know if |
@aaronsteers Implemented in 7baebb8 - |
|
@WillDaSilva @aaronsteers I've updated AJ's comment with my preferred text. Thanks for well thought out prompts @aaronsteers ! |
In #6862 we defered the activate of environments. The `Tracker` was implicitly relying on the environment being activated before it was instantiated, or no environment being activated at all. Unfortunately fixing that implicit dependency would be more effort than I'm willing to commit to this at this time. Instead, I have made it so that activating an environment will update `environment_name_hash` in the `ProjectContext` in the `Tracker`. New `ProjectContext` objects created past that point will, as before, get the active environment information from the `Project` object. I have verified that this works using Snowplow Micro locally. Before this change `environment_name_hash` was always `null`. After this change, `environment_name_hash` is set to the expected hashed string when an environment is active, and `null` when no environment is active. I would encourage at least one reviewer to double check this. Closes meltano/internal-data#51
Given the urgency of #6677, and the relative ease of #6834, I decided to quickly put together an implementation.
In this implementation, commands which require an environment call
activate_environment, and those which will only use an environment if it is explicitly provided (e.g. via--environment) callactivate_explicitly_provided_environment. By default no environment is required nor used, which comes with the side benefit of avoiding the "default environment activated" log message for most commands.Closes #6834