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

Update ca from 1.60 to 1.73 + Add debug info on ca repository #84

Merged
merged 6 commits into from
Nov 14, 2016

Conversation

guillaume-philippon
Copy link
Contributor

Every thing is on title

@@ -4,7 +4,7 @@ variable YUM_SNAPSHOT_NS ?= 'repository/snapshot';

@{
desc = namespace of template associated with CA RPM YUM snapshot
values = string
values = string
Copy link

Choose a reason for hiding this comment

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

trailing space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done with --amend option

# This work is licensed under the Creative Commons Attribution-Noncommercial
# 3.0 License. To view a copy of this license, visit
# 3.0 License. To view a copy of this license, visit
Copy link

Choose a reason for hiding this comment

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

remove trailing space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done with --amend option

@@ -18,10 +18,13 @@ default = YUM_SNAPSHOT_DATE
required = no
}
variable YUM_CA_RPM_SNAPSHOT_DATE ?= if ( is_null(YUM_CA_RPM_SNAPSHOT_DATE) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow this code. If YUM_CA_RPM_SNAPSHOT_DATE is not null or undef then this code will not run because of the ?= But if it is_null then you return SELF which will set it to undef. The addition of debug() statements seem to be a plaster over a block that should just be re-written to be less convoluted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just add some debug info. I will have a deep look in the code itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fix the useless code part of the template

@@ -37,8 +40,10 @@ include { 'quattor/functions/repository' };

'/software/repositories' = {
if ( is_defined(YUM_CA_RPM_SNAPSHOT_DATE) && (QUATTOR_RELEASE >= '14') ) {
debug(format('%s: adding CA repository for YUM snapshot %s',OBJECT,YUM_CA_RPM_SNAPSHOT_DATE));
add_repositories(SECURITY_CA_RPM_REPOSITORY,YUM_CA_RPM_SNAPSHOT_NS);
Copy link
Contributor

Choose a reason for hiding this comment

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

This checks and logs YUM_CA_RPM_SNAPSHOT_DATE but then adds YUM_CA_RPM_SNAPSHOT_NS to the repositories tree. Why not check and log YUM_CA_RPM_SNAPSHOT_NS instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YUM_CA_RPM_SNAPSHOT_NS is always defined. We must add this specific repository only if you use a QUATTOR_RELEASE >= '14' and SNAPSHOT_DATE is defined.

# This work is licensed under the Creative Commons Attribution-Noncommercial
# 3.0 License. To view a copy of this license, visit
# http://creativecommons.org/licenses/by-nc/3.0/
# Generated by ./builddist-egi.pl
# Generated by ./builddist-egi-multi.pl
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes in this file are OK. If the addition of debug is controversial/needs discussion then perhaps it should be a separate commit to allow it to proceed without being blocked by the questions above?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this file is generated by an external body (IGTF) and is just imported by us from the reference repo.

@jouvin jouvin added this to the 16.10 milestone Nov 8, 2016
@jouvin
Copy link
Contributor

jouvin commented Nov 8, 2016

@guillaume-philippon please could you rebase your PR so that we can include it in 16.10.

@guillaume-philippon
Copy link
Contributor Author

I merged with upstream

@jouvin jouvin merged commit 9b203d9 into quattor:master Nov 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants