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

Adding ETL related files for the appkernels module #28

Merged
merged 4 commits into from
Aug 31, 2017

Conversation

ryanrath
Copy link
Contributor

Description

Added the modules.d/appkernels.json file to the file_maps section of
build.json

Motivation and Context

So that the new file would be included in the packaged module / installed when 'install' is executed.

Tests performed

Manual test performed:

  • modified build.json w/ the new entry in file_maps/etc
  • executed: <xdmod_dir>/open_xdmod/build_scripts/build_package --module appkernels --skip-assets --verbose
  • cd <xdmod_dir>/open_xdmod/build
  • tar xvzf xdmod-appkernels-6.7.0.tar.gz
  • ls -alh configuration/modules.d/appkernels.json
    • ensure that the file exists
  • cat configuration/modules.d/appkernels.json
    • ensure that it is a json file and that it contains the correct information

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project as found in the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@@ -51,6 +51,8 @@ rm -rf $RPM_BUILD_ROOT
%{_libdir}/xdmod/
%{_datadir}/xdmod/
%{_docdir}/%{name}-%{version}__PRERELEASE__/
%{_docdir}/%{name}-%{version}/
%{_sysconfdir}/modules.d/appkernels.json
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong to me. Shouldn't it be %{_sysconfdir}/xdmod/modules.d/appkernels.json ?

@@ -51,6 +51,8 @@ rm -rf $RPM_BUILD_ROOT
%{_libdir}/xdmod/
%{_datadir}/xdmod/
%{_docdir}/%{name}-%{version}__PRERELEASE__/
%{_docdir}/%{name}-%{version}/
%{_sysconfdir}/modules.d/appkernels.json

%config(noreplace) %{_sysconfdir}/xdmod/*.d/appkernels.ini
Copy link
Member

Choose a reason for hiding this comment

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

Need to not have a wildcard here as it could mark modules.d/appkernels.json as a config noreplace (which is wrong)./

Added appkernels.json

- Added the modules.d/appkernels.json file to the file_maps section of
  build.json
- just making sure that the new module file: modules.d/appkernels.json is
  included in the rpm build. This would otherwise cause the build to fail.
- Explicitly cal out the module file as a regular file inclusion as opposed to
  config(noreplace).
- Add the other configuration files / directories as no-replace.

Testing consisted of building the rpm with the latest docker image and then
executing the following commands:
  - cd ~/rpmbuild/RPMS/noarch
  - rpm -qp --queryformat '[%{FILENAMES} %{FILEFLAGS} \n]'
    xdmod-appkernels-7.0.0-1.0.el7.centos.noarch.rpm |
    grep -E "appkernels\.*\.json" |
    grep /etc/xdmod
  - ex. output: /etc/xdmod/modules.d/appkernels.json 0
    - if the number is 17 then it's a config / noreplace
    - if the number is 0 then it's just a regular file.
  - you can find all the file flags and their values ( it's a bitflag ) @
    htttp://refspecs.linuxbase.org/LSB_4.1.0/LSB-Core-generic/LSB-Core-generic/pkgformat.html
@ryanrath
Copy link
Contributor Author

ryanrath commented Aug 8, 2017

@jpwhite4 I have updated the spec file. If you could take another look that would rock.

@@ -38,7 +38,8 @@
"configuration/roles.json": "roles.d/appkernels.json",
"configuration/setup.json": "setup.d/appkernels.json",
"configuration/rest.d": "rest.d",
"configuration/assets.d": "assets.d"
"configuration/assets.d": "assets.d",
"configuration/modules.d/appkernels.json": "modules.d/appkernels.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading this correctly, this specifies that configuration/modules.d/appkernels.json is mapped to etc/modules.d/appkernels.json, but I don't see configuration/modules.d/appkernels.json in this pull request or the 7.0 or 7.1 branches of xdmod-appkernels.

@@ -38,7 +38,8 @@
"configuration/roles.json": "roles.d/appkernels.json",
"configuration/setup.json": "setup.d/appkernels.json",
"configuration/rest.d": "rest.d",
"configuration/assets.d": "assets.d"
"configuration/assets.d": "assets.d",
"configuration/modules.d/appkernels.json": "modules.d/appkernels.json"
},
"etc/crond": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Refresh my memory, This places the mapped files into $INSTALL_ROOT/etc/cron.d, correct? But the block above it is etc and places files into $INSTALL_ROOT/etc/xdmod?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily, files in "etc/crond" are installed to whatever is specified to the --crondconfdir option of the install script. If an install root is specified by the --prefix option (and --cronconfdir is not specified) then those files are installed to PREFIX/etc/cron.d. For an RPM installation the --crondconfdir is specified as %{_sysconfdir}/cron.d where %{_sysconfdir} is typically /etc, so those files are placed in /etc/cron.d.

Likewise, the "etc" section destination is specified by the --sysconfdir option. For a source install using the --prefix option the default is PREFIX/etc (not PREFIX/etc/xdmod). For an RPM %{_sysconfdir}/xdmod is specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jtpalmer Thanks for the clarification

@@ -51,9 +51,14 @@ rm -rf $RPM_BUILD_ROOT
%{_libdir}/xdmod/
%{_datadir}/xdmod/
%{_docdir}/%{name}-%{version}__PRERELEASE__/
%{_docdir}/%{name}-%{version}/
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should be unnecessary since the line above it is intended to include the documentation files. If the pre-release versioning is broken please let me know.

%config(noreplace) %{_sysconfdir}/xdmod/assets.d/appkernels*.json
%config(noreplace) %{_sysconfdir}/xdmod/internal_dashboard.d/appkernels*.json
%config(noreplace) %{_sysconfdir}/xdmod/roles.d/appkernels*.json
%config(noreplace) %{_sysconfdir}/xdmod/setup.d/appkernels*.json
Copy link
Contributor

@jtpalmer jtpalmer Aug 25, 2017

Choose a reason for hiding this comment

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

Why is this line being replaced with four lines if nothing changed with these files?

Edit: I'm referring to line 56 being removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

If memory serves, @ryanrath wanted to take into account some sort of versioning or other appkernels files, but it looks like the same thing could be accomplished simply by changing appkernels.json to appkernels*.json

Copy link
Contributor Author

@ryanrath ryanrath Aug 28, 2017

Choose a reason for hiding this comment

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

@smgallo @jtpalmer The reason for the change is so that the modules.d/appkernels.json is included but not flagged as config(noreplace) while appkernels*.json in the other folders are flagged as config(noreplace).
The modules.d/<module>.json file is created during the module build step.

- per review comment by @jtpalmer
@ryanrath ryanrath merged commit 299de68 into ubccr:xdmod7.0 Aug 31, 2017
@tyearke tyearke mentioned this pull request Sep 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants