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: change cloud_cfg to Yaml and modify the source spec #3484

Merged
merged 0 commits into from
Sep 1, 2022
Merged

Conversation

xiangce
Copy link
Contributor

@xiangce xiangce commented Jul 27, 2022

Signed-off-by: Xiangce Liu [email protected]

All Pull Requests:

Check all that apply:

  • Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
  • Is this PR to correct an issue?
  • Is this PR an enhancement?

Complete Description of Additions/Changes:

@psachin
Copy link
Contributor

psachin commented Jul 28, 2022

If not urgent, I want understand this issue with @crackcodecamp before review.

@bfahr
Copy link
Contributor

bfahr commented Aug 3, 2022

@xiangce I'm concerned about this change because I think it might cause confusion between the the specs cloud_cfg which is filtered and cloud_cfg_filtered which is not a filtered spec but uses filters from cloud_cfg. I would say that this spec/parser is actually broken since it currently returns json instead of yaml, and because it does not retain the structure of the config file. Would you consider just fixing the current spec and parser instead of the current approach?

@xiangce
Copy link
Contributor Author

xiangce commented Aug 4, 2022

@bfahr - Thanks for reviewing.

Due to the current filter policy, the original content of the spec will be filtered per the allowlist by force when the spec is marked as filterable=True no matter if it's a legacy spec or datasource spec. This caused issue #3482 and resulted in the parser will never get the expected result specified in the filters, even after issue #3478 is fixed and the returned content is converted to Yaml.
The new added speccloud_cfg_filter is just a workaround, since currently, we cannot adjust the filter policy for datasource specs yet (I think we have discussed this in one of our sync meeting). Does it work if we add adequate comment/docstrings for the usage of the two specs?

@xiangce
Copy link
Contributor Author

xiangce commented Aug 23, 2022

@bfahr - I added some notes for the two specs in the docstring, please have a look.

@xiangce xiangce merged commit d42266a into master Sep 1, 2022
@xiangce xiangce deleted the fix_3478 branch September 1, 2022 02:04
xiangce added a commit that referenced this pull request Sep 1, 2022
* fix: change cloud_cfg to Yaml and modify the filtering target

- fix: #3482
- fix: #3478

Signed-off-by: Xiangce Liu <[email protected]>

* forget the tests
* fix doc test

Signed-off-by: Xiangce Liu <[email protected]>

* for python26 - 2
* add more doc for specs

Signed-off-by: Xiangce Liu <[email protected]>

* fix the doc tests

Signed-off-by: Xiangce Liu <[email protected]>
(cherry picked from commit d42266a)
xiangce added a commit that referenced this pull request Sep 6, 2024
* fix: change cloud_cfg to Yaml and modify the filtering target

- fix: #3482
- fix: #3478

Signed-off-by: Xiangce Liu <[email protected]>

* forget the tests
* fix doc test

Signed-off-by: Xiangce Liu <[email protected]>

* for python26 - 2
* add more doc for specs

Signed-off-by: Xiangce Liu <[email protected]>

* fix the doc tests

Signed-off-by: Xiangce Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants