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

fix: Handle case where setting metadata is None #6938

Merged
merged 5 commits into from Nov 1, 2022

Conversation

WillDaSilva
Copy link
Member

Before:

$ meltano config meltano set state_backend.uri file:///${MELTANO_ROOT_DIR}/.meltano/state
Need help fixing this problem? Visit http://melta.no/ for troubleshooting steps, or to
join our friendly Slack community.

'NoneType' object has no attribute 'is_redacted'

After:

$ meltano config meltano set state_backend.uri file:///${MELTANO_ROOT_DIR}/.meltano/state
Meltano setting 'state_backend.uri' was set in `meltano.yml`: 'file:////.meltano/state'

Instead of setting the non-existent setting, would we rather it error cleanly?

Closes #6937

Before:
```
$ meltano config meltano set state_backend.uri file:///${MELTANO_ROOT_DIR}/.meltano/state
Need help fixing this problem? Visit http://melta.no/ for troubleshooting steps, or to
join our friendly Slack community.

'NoneType' object has no attribute 'is_redacted'
```

After:
```
$ meltano config meltano set state_backend.uri file:///${MELTANO_ROOT_DIR}/.meltano/state
Meltano setting 'state_backend.uri' was set in `meltano.yml`: 'file:////.meltano/state'
```

Closes #6937
@netlify
Copy link

netlify bot commented Oct 26, 2022

Deploy Preview for meltano canceled.

Name Link
🔨 Latest commit b85de92
🔍 Latest deploy log https://app.netlify.com/sites/meltano/deploys/635abac0fa34a900084a9a5b

@WillDaSilva WillDaSilva marked this pull request as ready for review October 26, 2022 15:57
@tayloramurphy
Copy link
Collaborator

@WillDaSilva is the actual problem here that when you do a config for a setting that doesn't exist it gives this error? Looks like this is coming up because EDK things can use the config block however they want technically, right?

@WillDaSilva
Copy link
Member Author

is the actual problem here that when you do a config for a setting that doesn't exist it gives this error?

@tayloramurphy Yes. Whether or not we want to permit setting these settings is unknown to me, but we definitely shouldn't be erroring in this way.

I asked in the PR description:

Instead of setting the non-existent setting, would we rather it error cleanly?

If we do want it to error cleanly, I can update this PR to do so.

@tayloramurphy
Copy link
Collaborator

If we do want it to error cleanly, I can update this PR to do so.

@WillDaSilva I'd say have it execute but give a warning that "this setting is not known to Meltano".

@WillDaSilva
Copy link
Member Author

I'd say have it execute but give a warning that "this setting is not known to Meltano".

There's a nice parallel between that approach and what we do when a setting that does not exist is retrieved, which is emit a warning and return None.

@WillDaSilva WillDaSilva changed the title Handle case where setting metadata is None fix: Handle case where setting metadata is None Oct 27, 2022
@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #6938 (b85de92) into main (b830890) will decrease coverage by 0.02%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main    #6938      +/-   ##
==========================================
- Coverage   88.75%   88.73%   -0.03%     
==========================================
  Files         284      284              
  Lines       20548    20542       -6     
  Branches     2028     2027       -1     
==========================================
- Hits        18237    18227      -10     
- Misses       1976     1979       +3     
- Partials      335      336       +1     
Impacted Files Coverage Δ
src/meltano/cli/interactive/config.py 30.36% <71.42%> (+0.73%) ⬆️
src/meltano/core/settings_service.py 97.43% <100.00%> (-0.09%) ⬇️
src/meltano/cli/utils.py 82.74% <0.00%> (-1.06%) ⬇️
src/meltano/core/plugin_discovery_service.py 88.75% <0.00%> (-0.60%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@WillDaSilva WillDaSilva merged commit f3a9486 into main Nov 1, 2022
@WillDaSilva WillDaSilva deleted the 6937-setting-metadata-is-none branch November 1, 2022 13:40
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.

bug: Error when setting metadata is None
3 participants