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

Execute external script(s) on firstboot #44

Merged
merged 20 commits into from
May 20, 2019

Conversation

illuusio
Copy link
Contributor

As in Issue #4 states: There should be way to execute external scripts. This is bit naive way to achieve that goal but should be enough for most cases.

@Vogtinator
Copy link
Member

That's indeed a naive way, those scripts aren't run at all if firstboot is run in dry-run mode.

This addition would be a externally visible API, so IMO we should make it slightly more refined before making it available.

@illuusio
Copy link
Contributor Author

Yes but nothing is run in dry-run? So I though that would be as it should 💃.
Just curious what do you mean visible externally visible API? Do you meand SELinux?

@Vogtinator
Copy link
Member

Yes but nothing is run in dry-run? So I though that would be as it should.

Not quite right - it still shows dialogs and computes the commands, even if they're not executed. That makes it possible to test it without making changes to the running system.

IMO something like this should be implemented by sourcing (instead of executing) files in the specified directories (I'd say /etc/jeos-firstboot in addition as well, for custom scripts not in official packages). That would need to be documented, along with all variables and functions the scripts can use (like run and d).

Just curious what do you mean visible externally visible API? Do you meand SELinux?

If jeos-firstboot calls external scripts, that's an API. jeos-firstboot changes in the future shouldn't break those scripts in a backwards-incompatible way.

@illuusio
Copy link
Contributor Author

illuusio commented Mar 1, 2019

Ok this can be closed I suppose and rethinked?

@Vogtinator
Copy link
Member

We can leave this open and discuss here as well, what do you prefer?

@illuusio
Copy link
Contributor Author

illuusio commented Mar 1, 2019

Ok we can do that also. This is needed feature for me.

@Vogtinator
Copy link
Member

@illuusio
Copy link
Contributor Author

illuusio commented Mar 4, 2019

I could be all wrong but should it be something like this (I'll draft this on wiki after this if needed):

  • Source script with --dry if needed (i.e . script --dry) or have global variable for this. Because they are sources they should allow using function d, run and others.
  • If script returns 0 it's success and 1 if not. Should everything be stopped after error?
  • There could be need for functions like (Names can be anything): before(), execute() and after(). These could be called from master script and not just trust it will be executed or is just KISS exceute script and be happy.
  • Could they return new global variables?

@Vogtinator
Copy link
Member

Yeah, using hook functions is probably the best approach here.

I'm thinking about something like:

# Load all available modules
pushd /usr/share/jeos-firstboot
for module in *; do
    source $module && modules+=("$module")
done
popd

...
# Run all systemd_firstboot hooks
for module in "${module[@]}"; do
    [ $(type -f "${module}_systemd_firstboot") = "function" ] || continue
    "${module}_systemd_firstboot"
done

@illuusio
Copy link
Contributor Author

illuusio commented Mar 4, 2019

Ok now we are talking about serious BASH wizarding 👍. How do to write that in Wiki?

@Vogtinator
Copy link
Member

I added some ideas to the page - if you have anything else, just add it.

@illuusio
Copy link
Contributor Author

illuusio commented Mar 6, 2019

I made changes to have more advanced version of script launching. It should more like what has risen in this discussion and wiki page.

files/usr/lib/jeos-firstboot Outdated Show resolved Hide resolved
files/usr/lib/jeos-firstboot Outdated Show resolved Hide resolved
files/usr/lib/jeos-firstboot Show resolved Hide resolved
files/usr/lib/jeos-firstboot Outdated Show resolved Hide resolved
files/usr/lib/jeos-firstboot Outdated Show resolved Hide resolved
@illuusio
Copy link
Contributor Author

illuusio commented Mar 7, 2019

Now should be reviews updated

files/usr/lib/jeos-firstboot Outdated Show resolved Hide resolved
@Vogtinator Vogtinator requested a review from lnussel March 8, 2019 12:26
@Vogtinator
Copy link
Member

Looking good so far (except the broken || true now), so only actually defining and using hooks needs to be done.

I guess we should define the format arguments can be supplied to systemd firstboot as a global systemd_firstboot_args variable directly.

@illuusio
Copy link
Contributor Author

All global variables should be available because they are run under main script? Should I turn that WIFI stuff as a script for a test so this can be evaluated to work?

@Vogtinator
Copy link
Member

Should I turn that WIFI stuff as a script for a test so this can be evaluated to work?

Yes, that would be a good example and testcase.

@illuusio
Copy link
Contributor Author

Ok I turned WIFI to script. I have to make RPM from this that I can test it on image.

files/usr/lib/jeos-firstboot Outdated Show resolved Hide resolved
Copy link
Member

@Vogtinator Vogtinator left a comment

Choose a reason for hiding this comment

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

Looking good so far, just some cleanups in the first module missing.

It would work fine already, but as the first user of this functionality will serve as an example, it should be a good one IMO.

config_wireless=true
fi
fi
if [ "$config_wireless" = "true" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

The whole "if" can be moved one layer up, like this:

if foo; then
   bar;
fi

to

foo || return
bar

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'm not sure that it's working as expected. Should there be some == or something to make sure it's working before ||

files/usr/share/jeos-firstboot/raspberrywifi Outdated Show resolved Hide resolved
files/usr/share/jeos-firstboot/raspberrywifi Outdated Show resolved Hide resolved
@Vogtinator
Copy link
Member

Can you also add some short comments on each hook implementation in the module, e.g.

# This is called by jeos-firstboot for user interaction and access to the global systemd_firstboot_args array
raspberrywifi_systemd_firstboot() {
...
}

run mv -f $config_file /etc/sysconfig/network/ifcfg-$wlan_device
}

# This is called by jeos-firstboot for user
Copy link
Member

Choose a reason for hiding this comment

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

Nope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope for comment?

Copy link
Member

Choose a reason for hiding this comment

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

Yup - the comment for _post should be different than the one for _systemd_firstboot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok now there is very cryptic comment

Copy link
Member

Choose a reason for hiding this comment

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

This comment doesn't make much sense. What about

# This is called after the configuration steps finished successfully

files/usr/share/jeos-firstboot/raspberrywifi Outdated Show resolved Hide resolved
@Vogtinator
Copy link
Member

Almost there! Next week @gmoro is back and will have a look as well.

@illuusio
Copy link
Contributor Author

Hopefully. This has been serious lesson of Bash-fu. I though I knew something but learning is good thing.

Copy link
Member

@Vogtinator Vogtinator left a comment

Choose a reason for hiding this comment

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

Looking good, except for the comment.

Just a heads up: #46 would cause a conflict with this PR as it makes the raspberrywifi_post function unnecessary. So depending in which order the PRs are merged, it might need a rebase.

run mv -f $config_file /etc/sysconfig/network/ifcfg-$wlan_device
}

# This is called by jeos-firstboot for user
Copy link
Member

Choose a reason for hiding this comment

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

This comment doesn't make much sense. What about

# This is called after the configuration steps finished successfully

@Vogtinator Vogtinator requested a review from gmoro March 22, 2019 09:53
@illuusio
Copy link
Contributor Author

I'll rebase if needed

@aplanas
Copy link
Contributor

aplanas commented May 8, 2019

I like this one!

Is there a plan to provide a list of snippets that can be skipped? In that way we can refactor jeos-firstboot in pieces, living in /usr/share/jeos-firstboot, and using some env variable tailor the list of snippets that will be executed.

Another option is to have some enabled.d directory, with links to the snippets that will be executed. The initial RPM can enable a selection of them, and later via config.sh we can change the final list.

@Vogtinator
Copy link
Member

I'd prefer if every module was split into a package with proper dependencies (e.g. WLAN configuration needs wifi tools) and only those modules actually used are installed. config.sh could remove modules it doesn't need.

Having a configuration file referenced via EnvironmentFile=-/etc/jeos-firstboot.conf would work as well, it would also allow setting some of the other exposed options.

@aplanas
Copy link
Contributor

aplanas commented May 8, 2019

Having a configuration file referenced via EnvironmentFile=-/etc/jeos-firstboot.conf would work as well, it would also allow setting some of the other exposed options.

Even better. I agree.

@illuusio
Copy link
Contributor Author

illuusio commented May 9, 2019

So should that /etc/jeos-firstboot.conf contain enviromental variables which are read on systemd startup? Would it be just like JEOS_FIRSTBOOS_SCRIPTS="script1.sh script2.sh" or something fancier?

@lnussel lnussel removed their request for review May 13, 2019 09:31
@lnussel
Copy link
Member

lnussel commented May 13, 2019

can't really spend time reviewing this atm but this clearly goes into a directory of systemd style handling of services.

Copy link
Collaborator

@gmoro gmoro left a comment

Choose a reason for hiding this comment

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

Looks good.

@Vogtinator Vogtinator merged commit 33a35a9 into openSUSE:master May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants