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.

Sign up for GitHub

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

Jump to bottom

Fixes #23039: Add rich form types to yaml techniques #4887

Merged
merged 1 commit into from Jul 20, 2023
Merged

Fixes #23039: Add rich form types to yaml techniques #4887

merged 1 commit into from Jul 20, 2023

Conversation

amousset
Copy link
Member

@amousset amousset commented Jul 11, 2023 •
edited

https://issues.rudder.io/issues/23039

For the most part, adds support for all directive form niceties supported in the built-in techniques.

Screenshot from 2023-07-19 15-06-48

A few interesting points:

  • It does not stick to the model used in the metadata.xml. Particularly we use types as much as possible, and less constraints. Some values were renamed for either consistency with what is already there or just clarity.
  • The escaping behavior is not part of the type but an independent parameter, allowing more flexibility (and hopefully less confusion).
  • Select and regex parameter types have been merged with the ones used for method parameters, in order to converge iteratively.
  • Removes ParameterType (scala)/Escaping (rust) from technique parameters, this was a mistake we only have them on method parameters for now.
  • Allow omitting ids: there are generating on the fly during compilation. Also adding a command allowing to add ids to an existing files (though for now it removes all formatting/comments).

@amousset amousset requested a review from Fdall July 11, 2023 08:05
@amousset amousset marked this pull request as draft July 11, 2023 08:06
@amousset amousset added the WIP Use that label for a Work In Progress PR that must not be merged yet label Jul 11, 2023
@amousset
Copy link
Member Author

PR updated with a new commit

6 similar comments
@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset amousset changed the base branch from master to branches/rudder/8.0 July 19, 2023 00:10
@amousset
Copy link
Member Author

PR updated with a new commit

4 similar comments
@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset amousset removed the WIP Use that label for a Work In Progress PR that must not be merged yet label Jul 19, 2023
@amousset amousset marked this pull request as ready for review July 19, 2023 13:50
@amousset
Copy link
Member Author

PR updated with a new commit

6 similar comments
@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

Fdall
Fdall reviewed Jul 20, 2023
View reviewed changes
policies/rudderc/tests/cases/general/escaping/metadata.xml Outdated
<CONSTRAINT>
<TYPE>textarea</TYPE>
<MAYBEEMPTY>true</MAYBEEMPTY>
<TYPE>string</TYPE>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change from textarea to string here?

Copy link
Member Author

@amousset amousset Jul 20, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because previous code used to put textarea everywhere fro no reason

@amousset
Copy link
Member Author

PR updated with a new commit

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/4887
-- Your faithful QA
Kant merge: "Two things awe me most, the starry sky above me and the moral law within me."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/71157/console)

@amousset
Copy link
Member Author

OK, squash merging this PR

@amousset amousset force-pushed the bug_23039/add_rich_form_types_to_yaml_techniques branch from 4b1d8f6 to 2fefd25 Compare July 20, 2023 12:03
@amousset amousset merged commit 2fefd25 into Normation:branches/rudder/8.0 Jul 20, 2023
0 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fdall Fdall Fdall approved these changes

Assignees
No one assigned
Labels
qa: Can't merge Ready for merge
Projects
None yet
Milestone
No milestone
3 participants
@amousset @Normation-Quality-Assistant @Fdall