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

Match files in /var/lib/cobbler/autoinstall_scripts only #1811

Closed
wants to merge 1 commit into from

Conversation

dincamihai
Copy link

This PR restricts matching script_name with files found in /var/lib/cobbler/autoinstall_scripts only.

PS: this is my first PR here so please be gentle :)

@@ -1063,9 +1063,16 @@ def generate_script(self, what, objname, script_name):
else:
blended['img_path'] = os.path.join("/images", distro.name)

template = os.path.normpath(os.path.join("/var/lib/cobbler/autoinstall_scripts", script_name))
Copy link
Author

Choose a reason for hiding this comment

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

When generate_scripts was introduced (I believe here: 955568d#diff-7ae3873d5c766467d4634ede08a8e6d6R1037) the path was /var/lib/cobbler/scripts and it actually existed. The new path /var/lib/cobbler/autoinstall_scripts does not exist.
Is this intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact there still are another references to /var/lib/cobbler/autoinstall_scripts
MANIFEST.in:recursive-include autoinstall_scripts *
setup.py: ("%s/scripts" % libpath, glob("autoinstall_scripts/*")),
Without checking the whole git history.., it looks as if a setup.py install should create the directory or the .spec file and autoinstall_scripts (former kickstart_ functionality naming) has been split out from other scripts?
Not sure, but if, then the other autoinstall_scripts occurences should be taken care for too, to aovid other leftovers.

BTW: What is your problem/error?
Maybe simply creating the directory solves things. Then the correct fix might be to create the dir in the .spec file?

@jmaas
Copy link
Member

jmaas commented Sep 3, 2017

Hi ! Sorry for the late response.

But /var/lib/cobbler/scripts does exist after a fresh install.
The path in tftpgen.py has mistakenly been altered.

@jmaas jmaas self-requested a review September 3, 2017 06:55
@dincamihai
Copy link
Author

@jmaas should I change the path in tftpgen.py to /var/lib/cobbler/scripts?
Are you happy with the rest of the PR?

@brejoc
Copy link
Member

brejoc commented Jan 10, 2019

@dincamihai Could you rebase this on the futurize branch please?

Copy link
Member

@jmaas jmaas left a comment

Choose a reason for hiding this comment

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

Please do change the path then we can get this into the tree.

@SchoolGuy SchoolGuy added this to Pull Requests in Backlog Aug 15, 2019
@SchoolGuy
Copy link
Member

@dincamihai Could you please rebease and we then see if this works?

@watologo1
Copy link
Contributor

I also agree that autoinstall_scripts is much nicer than scripts.
Unfortunately a lot has changed since then. And one has to be really careful to find all occurences.
I close this one as rebasing is more or less a re-doing the patch.
Also, this is a cleanup and not a bug fix. It is nice to have (and I am willing to look at it and merge it if it is rebased to latest HEAD code base). If you re-do, please be careful and try hard to find every code/readme/whatever where this "script" or maybe there are still even older/outdated definitions in the docs and re-name all of them.

@watologo1 watologo1 closed this Oct 31, 2019
@SchoolGuy SchoolGuy moved this from Pull Requests to Done in Backlog Nov 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants