-
Notifications
You must be signed in to change notification settings - Fork 9
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
Separate out potentially useful type lists and variant utility functi… #377
Conversation
…ons to separate header files that could be used downstream w/o needing the variant definition itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve of exposing this machinery. Just need to make sure tests pass. Probably merits a changelog entry too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! And good timing! I think we'll probably be using this here soon (i.e. months) in some of the integration work
…ools and helper functions out of the header where variant is defined.
.gitlab-ci.yml
Outdated
@@ -40,6 +40,8 @@ default: | |||
id_tokens: | |||
SITE_ID_TOKEN: | |||
aud: https://asc-git.lanl.gov | |||
CI_JOB_JWT: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rbberger is this change still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no other pipeline or project that needed that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the change that the CI was asking for on other projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused---do we need this change or no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I don't think so. My theory is that re-git switched over to the new version (the thing that they were warning for many months), which deprecated CI_JOB_JWT
and that there was some outdated runners still floating around while me and Karen were out. The outdated runner would then complain about not getting the no longer provided token. All it needed was to get that runner updated to accept the new SITE_ID_TOKEN
, which seems to have eventually happened.
after comments are addressed by @rbberger we can merge @Yurlungur |
I will need to check gitlab ci again now that I removed the ci changes. |
This is passing gitlab ci, clicking the button. |
…ons to separate header files that could be used downstream w/o needing the variant definition itself.
PR Summary
@Yurlungur @jhp-lanl @gopsub @jdolence
This PR includes changes that separates potentially useful type lists and variant utility functions into separate header files that could be used by downstream codes without needing to include the variant type itself. It allows one to start with the final monolithic list of eos types (or any of the intermediate type lists) and build from there using concat, transform etc. Then the final variant's type can be constructed using the variant utility functions.
PR Checklist
make format
command after configuring withcmake
.If preparing for a new release, in addition please check the following: