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

Add string comparison functions to Go template executors #4560

Merged

Conversation

ianyong
Copy link
Contributor

@ianyong ianyong commented Oct 24, 2023

Proposed changes

Add the following helper functions to the Go template executors to enable string comparisons:

Not very sure why there are two versions of the template executors. I presume version1 will eventually be phased out in favour of version2 given the naming? Either way, I've made the same changes to both versions.

Resolves #4555.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

These string comparison functions come from Go's standard library,
so there isn't really a need to test them extensively. Instead,
these test cases are mainly to assert that these functions can be
correctly used within the template executor.
@ianyong ianyong requested review from a team as code owners October 24, 2023 15:26
@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Oct 24, 2023
Copy link
Contributor

@jjngx jjngx left a comment

Choose a reason for hiding this comment

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

Thank you for contributing! Could you please keep tests in the same style as in other packages in the project?

  • call t.Parallel() as a first line in a test
  • use anonymous struct literals for table tests (as in other test files)
  • keep test struct fields not exported

* Call `t.Parallel()` as the first line in each test case.
* Use anonymous struct literals for table tests.
@ianyong
Copy link
Contributor Author

ianyong commented Oct 24, 2023

@jjngx Thanks for reviewing! I've made the changes for the first 2 points. For the last point, the test struct fields need to be exported for them to be accessed within the templates, else the tests will fail with <struct field name> is an unexported field of struct type <struct shape>. It's also why the expected struct field is not exported across all the test cases - because it does not need to be accessed within the templates.

@ianyong ianyong requested a review from jjngx October 24, 2023 17:22
Copy link
Contributor

@jjngx jjngx left a comment

Choose a reason for hiding this comment

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

@ianyong thank you again for contributing! 🚀

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #4560 (a1515a1) into main (14673e9) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4560      +/-   ##
==========================================
+ Coverage   51.97%   51.99%   +0.01%     
==========================================
  Files          59       59              
  Lines       16962    16960       -2     
==========================================
+ Hits         8816     8818       +2     
+ Misses       7849     7847       -2     
+ Partials      297      295       -2     
Files Coverage Δ
internal/configs/version1/template_helper.go 100.00% <ø> (ø)
internal/configs/version2/template_helper.go 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@jjngx
Copy link
Contributor

jjngx commented Oct 24, 2023

@ianyong could you please update and push your branch?

@jjngx jjngx requested review from a team, vepatel, haywoodsh and shaun-nx October 24, 2023 18:51
@ianyong
Copy link
Contributor Author

ianyong commented Nov 1, 2023

Is there anything I can do to get this PR merged? The failing CI jobs don't seem to be caused by my changes, or at least I see other PRs with the same failed jobs being merged.

@vepatel
Copy link
Contributor

vepatel commented Nov 1, 2023

@hanyouqing @ianyong we'll prioritise merging this in coming week, thanks! You can monitor the state from parent issue.

@jjngx
Copy link
Contributor

jjngx commented Nov 3, 2023

@ianyong could you please update and push your branch?

@jjngx
Copy link
Contributor

jjngx commented Nov 7, 2023

@ianyong could you please update and push your branch?

Copy link
Contributor

@shaun-nx shaun-nx left a comment

Choose a reason for hiding this comment

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

👍

@jjngx jjngx merged commit 9a52e02 into nginxinc:main Nov 7, 2023
62 of 65 checks passed
@shaun-nx
Copy link
Contributor

shaun-nx commented Nov 7, 2023

Thank you again @ianyong for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add string comparison Go template functions
5 participants