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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore unknown root elements starting with x- #1379

Merged
merged 9 commits into from May 24, 2020
Merged

Ignore unknown root elements starting with x- #1379

merged 9 commits into from May 24, 2020

Conversation

jetersen
Copy link
Member

This should help #1246 to use anchors on root level.

Added tests for unknown root elements and added an alias test for EC2 cloud to demonstrate the potential.

cc @v1v @timja @almiroshnich

fixes #1246

Your checklist for this pull request

馃毃 Please review the guidelines for contributing to this repository.

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or in Jenkins JIRA
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Did you provide a test-case? That demonstrates feature works or fixes the issue.

Copy link
Member Author

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

This would properly need some docs

integrations/pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Needs docs and 1 question on naming, but nice!

@codecov
Copy link

codecov bot commented May 12, 2020

Codecov Report

Merging #1379 into master will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1379      +/-   ##
============================================
+ Coverage     79.42%   79.61%   +0.19%     
- Complexity      797      804       +7     
============================================
  Files            66       66              
  Lines          2323     2330       +7     
  Branches        323      325       +2     
============================================
+ Hits           1845     1855      +10     
+ Misses          377      375       -2     
+ Partials        101      100       -1     
Impacted Files Coverage 螖 Complexity 螖
...a/io/jenkins/plugins/casc/ConfigurationAsCode.java 75.47% <100.00%> (+1.31%) 101.00 <5.00> (+7.00)

@jetersen jetersen added the feature A PR that adds a feature - used by Release Drafter label May 12, 2020
@jetersen
Copy link
Member Author

@almiroshnich any suggestion on how to document this feature?

@sun-mir
Copy link

sun-mir commented May 12, 2020

@jetersen huge kudos to you for doing this 馃殌

As for the documentation subject, I don't feel myself competent enough here to make suggestions, but if you'd ask me directly - I'd put a note about such functionality under jenkinsci/configuration-as-code-plugin//README.md#initial-configuration (after unclassified screenshot, before Examples section). Such a note could have a reference to the official YAML Node Anchors documentation and a few links to the existing JCasC demos (have to be adjusted as well). One could be easily taken from your test here or from my GH issue. Probably, yet another reasonable use of such functionality with anchors might be for permanent build agents, but practically speaking it's pretty much the same as other cloud-templates, either eC2Fleet or kubernetes...

@v1v
Copy link
Member

v1v commented May 19, 2020

neat! From the docs point of view, in the main README.md, I think it could be added as an example of using the node anchors, to be automatically validated with the annotation: @ConfiguredWithReadme("README.md#id") as done in the https://github.com/jenkinsci/configuration-as-code-plugin/blob/master/integrations/src/test/java/io/jenkins/plugins/casc/TopReadmeTest.java.

@jetersen
Copy link
Member Author

jetersen commented May 24, 2020

as a FYI, we use snakeyaml at the moment which is a processor for YAML 1.1 spec.

@jetersen
Copy link
Member Author

I would like to avoid linking to the yaml spec as we do not follow the spec 100% for instance we do not support maps.

@jetersen
Copy link
Member Author

As this has full test coverage and blocking #1375 I am going ahead and merging it.

@jetersen jetersen merged commit 1dba3f8 into jenkinsci:master May 24, 2020
@jetersen jetersen deleted the fix/1246 branch May 24, 2020 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A PR that adds a feature - used by Release Drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support YAML syntax features: Anchors, Aliases and Extensions
4 participants