-
Notifications
You must be signed in to change notification settings - Fork 18
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
Plenary template adjustment to fix the template library support #87
Conversation
@urbonegi I just updated the PR based on our discussion. I added another change I had in mind (I mentioned it during our discussion but it was may not clear) to allow to define that a variable is equal to another variable and I used this to define |
Hi, just noticed that all the commits in this PR have different change-id. Can you please change this to have the same change id? Thanks |
@Shaeli Done! Sorry it was not clear for me if there should be one |
Please ignore the test failures, these are an artifact of the move from travisci.org to travisci.com |
@Shaeli I just fixed a typo in the second commit message... (missing |
I have just re-enabled branch protection for upstream and master, please let me know if it causes any problems pushing to the repo. |
lib/aquilon/aqdb/model/building.py
Outdated
@@ -34,8 +34,4 @@ class Building(Location): | |||
|
|||
address = Column(String(255), nullable=False) | |||
|
|||
next_rackid = Column(Integer, nullable=False, default=2) |
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.
Added by accident? Probably you need to rebase on latest upstream.
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.
This is a little bit strange, I thought I just reverted the previous commit which had something related to building.py. I started from the latest upstream if I'm right (latest=1 week ago!). May be the easiest is that I remove this line (or building.py) from the revert commit if this is the only change?
@jouvin so we do not understand fully why the archetype/base does not indeed become the first one if one would skip loading the pan/unis and functions? Please see my comment here:#86 Another this is that, I advised @Shaeli that we do need same 'change-id' for all the commits in the PR - while it is not true. I got confused sorry. The change-id needs to be different for each commit. It is there so that gerrit can cope with rebase, when commit-id will change - change-id will not :) sorry for confusion! |
@urbonegi I hope that #86 (comment) answers your question! It seems that MS has some concerns (that I don't really understand frankly) about these changes, because they are not useful for MS who does not rely on the template library. To move forward with this PR asasp, as it is critical for allowing the use of Aquilon with the template library without hacking Aquilon, I propose to add a new config option, About Gerrit Change-ID, don't worry. This is easy to generate again a separate one for each commit! |
@urbonegi I implemented what I suggested in my previous comment, with even more elementary commits (support for variable definition based on a variable and addition of the new variable |
f19fbc0
to
d88cf72
Compare
After the very long and intense discussion with @ned21 in #86 (😄), I remove from this PR the 2 commits not related to the template library support but to the addition of an
|
For the record, I have no strong preferences for the name, as long as it is clear what it is for. Ours currently looks like this: declaration template archetype/preface;
final variable ARCHETYPE = "ral-tier1";
final variable QUATTOR_RELEASE = '17.12.0';
variable LOADPATH = prepend(SELF, format('template-library/%s/core', QUATTOR_RELEASE));
variable LOADPATH = prepend(SELF, format('template-library/%s/standard', QUATTOR_RELEASE));
variable LOADPATH = prepend(SELF, 'ral-tier1/core');
variable LOADPATH = prepend(SELF, 'ral-tier1/standard'); |
etc/aqd.conf.defaults
Outdated
@@ -345,8 +345,6 @@ gzip_output = false | |||
# Assume the webserver will decompress transparently as needed. | |||
# only used if gzip_output = true | |||
transparent_gzip = true | |||
template_extension = .tpl |
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.
why this is here?
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.
A mistake of mine! Thanks for noticing it.
Ok, this been on-going for some time now. We are happy to merge this one nearly as it is. We suggest to have code like this in hosts, clusters and metaclusters before the base/archetype temaple:
This new 'archetype/declarations' template should accommodate @jrha suggestion above + could contain pan/units and pan/functions. MS later could also move to it too, but for now it could be controlled with an option. |
@urbonegi I think @ned21 mentioned that but also agreed later that it should be discussed separately. I don't see any reason to couple them and at this stage, I am not convinced this is a good move as it requires the user to do more when for me, the objective is to have the user doing only the things that the broker cannot do in a sensible manner (or without too much complexitiy). |
I updated the PR taking into account most of @urbonegi remarks. 2 points still need to be agreed:
|
re: The name My view is that we cannot stop people making changes other than LOADPATH manipulations so we should not name it something that me become laughable false in a few years as people find other clever uses for it. Hence "declarations" is exactly what it says it is: you can use it for the same things that PAN defines as as declaration, anything else and your template will be invalid. And that's enforced in the compiler. The config option name: Removal of pan/units,functions: I suggested this because those lines are now superfluous and can be more accurately managed per-archetype in the preface template. In particular for us it means that for those those archetypes where we do not use the definitions in those files, we can remove the lines altogether which we regard as a nicely improvement. It also makes the object template much cleaner by removing a hardcoded dependency on files from the TL that are not relevant to many of our archetypes. |
@ned21 we are not in strong disagreement on the reasoning, despite we have not yet converged on the conclusion 😄
|
@ned21 if you agree, i think we could go one step at the time. For now just allow to add the extra initial template if newly defined config option is set to true. Then later we might move the functions and units there. However, calling that new template 'archetype/loadpath' is to explicit. It locks the usage of this template for one usecase - define the loadpath var. However, lets not limit ourselves like that. Please change the name to 'archetype/declarations' or 'archetype/preface' as suggested above. the config option then should be 'object_declarations_template' or 'object_preface_template'. I vote for 'declarations'. @jouvin there are 2 small other issues in the PR -will add comments. Hopefully we will have an agreement with this - it been ongoing for too long ;) |
etc/aqd.conf.defaults
Outdated
@@ -345,6 +345,9 @@ gzip_output = false | |||
# Assume the webserver will decompress transparently as needed. | |||
# only used if gzip_output = true | |||
transparent_gzip = true | |||
template_extension = .tpl |
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.
it would make sense to fix the 'Revert' commit to not delete this rather than re-add it in here.
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.
You mean that this line disappeared because of the revert commit? This is stange... That means that the original commit did some bad things! I'll double check and fix the revert commit in this case...
# Included only if template_library_configured option is true | ||
# This template is optional: included only if it exists | ||
if self.config.getboolean("panc", "object_preface_template"): | ||
pan_include(lines, "archetype/loadpath") |
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 think you missed: lines.append("") after pan_include
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 thought you asked me to delete it in a previous comment.. I'll readd it!
lib/aquilon/worker/templates/host.py
Outdated
# Included only if template_library_configured option is true | ||
# This template is optional: included only if it exists | ||
if self.config.getboolean("panc", "object_preface_template"): | ||
pan_include(lines, "archetype/loadpath") |
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 think you missed: lines.append("") after pan_include
# Included only if template_library_configured option is true | ||
# This template is optional: included only if it exists | ||
if self.config.getboolean("panc", "object_preface_template"): | ||
pan_include(lines, "archetype/loadpath") |
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 think you missed: lines.append("") after pan_include
- Except lib/aquilon/aqdb/model/building.py that should not have been part of the initial commit This reverts commit 1ada34b. Change-Id: I32e0681660f53db630cbcddf0163cb4e694d9ade
@urbonegi hopefully the last version of the commits should fix all your remaining remarks. Sorry for overlooking that the |
Sure. Where does that leave the code we added last year as an unsuccessful attempt to fix the same problem? If no one is using that, can we repurpose it to remove those options as a separately configurable item? Am I correct in thinking that at present the option replaces the include with an include { if_exists } ? |
@ned21 yes the commit adding the |
OK, so if it's reverted that's good. Re-adding it as include_pan_core_templates and have it not include them at all (not even with if_exists) is fine with me but I shall defer final judgement @urbonegi since she made a very good point about separation of concerns and tackling one thing in each PR. |
So I think we all agree eventually! It was me who suggested first to separate both issues so I agree with @urbonegi remark on this point 😄 It'd be good if you (@ned21 ) could open another issue to tackle the pan core templates include. If @urbonegi agrees with the current state of this PR, you have the green light from my side to merge it after the internal review. |
lines.append("") | ||
|
||
# Okay, here's the real content | ||
pan_include(lines, ["pan/units", "pan/functions"]) |
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 am really confused to see this happened: hosts.py have the lines.append("") after the pan_include where cluster.py and metacluster.py do not...
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.
@urbonegi the easiest is that you tell me what is your expectation for blank lines. My original PR was one after the include of declarations.pan
and one after the include of pan core templates but you commented, if I understood properly, that it was too much. Is what is currently in host.py
what you want everywhere?
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 see now that the host.py was inconsistent in blank lines initially (1ada34b) - so that is fine. Revert here without changing is fine.
etc/aqd.conf.lal
Outdated
@@ -0,0 +1,150 @@ | |||
# This config file is for running the broker outside of MS where no afs available. |
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.
What is this :) ?
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.
Another mistake of mine, should not be part of the commit....
- Only if object_declarations_template option is true: default=false (true in aqd.conf.noms) - Template must be archetype/declarations (intended to define LOADPATH mainly) - It is the first executed template in the plenary object - Implemented for host, cluster and metacluster Fixes quattor#86 Change-Id: I7e57401fc82bfbc5af21d3d07efe543e999c23e6
@urbonegi I had another look to the commit, saw that the comment was still refering to |
@Shaeli it looks good now - ready to be fetched for review/internal CI. If tests succeed we can release this one next week. |
Great! |
merged: 3ccb231 |
…merge/master/by_topic/serviceaddr_to_sn to master * commit '2431e18e14bcffb83e2ff380f320a653e251aef8': Allow service address PTR to point at shared name Refactor add_address_alias into dbwrappers/dns.py Fix minor formatting bug in notification error
For host, cluster and metacluster plenary templates, include a preface template, called
archetype/declarations.pan
, intended mainly to define the loadpath for the rest of the profile. This happens only if the broker config optionobject_declarations_template
istrue
(false
by default).Broker option
include_pan
is also removed (addition reverted), as it is superseded by the previous changeThis PR is critical for enable the use of the template library in Aquilon
Fixes #86