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.

Sign up for GitHub

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

Jump to bottom

Doc fixes #105

Merged
merged 1 commit into from
Aug 23, 2018
Merged

Doc fixes #105

merged 1 commit into from
Aug 23, 2018

Conversation

dmatetelki
Copy link
Contributor

READMEs:

  • Installing again the once-installed README.rst and README.packaging

VCLs:

  • Installing the commented-out builtin.vcl to /etc/varnish, just like
    the default.vcl
  • Not installing these 2 anymore to /usr/share/doc/varnish
  • A non-commented version of builtin.vcl is installed into
    /usr/share/doc/varnish/examples.

@dmatetelki dmatetelki requested a review from dridi August 21, 2018 14:57
@dmatetelki
Copy link
Contributor Author

dmatetelki commented Aug 21, 2018 •
edited by dridi
Loading

Maybe VC should not try to install builtin.vcl and example.vcl into doc in the first place?
https://github.com/varnishcache/varnish-cache/blob/master/etc/Makefile.am#L5-L6

TODO:
backport to 6.x branch

dridi
dridi reviewed Aug 21, 2018
View reviewed changes
debian/rules Outdated
@@ -47,13 +47,15 @@ override_dh_auto_install:
dh_auto_install -a
install -d debian/tmp/etc/varnish
install -T -m 0644 etc/example.vcl debian/tmp/etc/varnish/default.vcl
install -T -m 0644 etc/builtin.vcl debian/tmp/etc/varnish/builtin.vcl
Copy link
Member

Choose a reason for hiding this comment

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

I think builtin.vcl belongs in /usr/share/doc/varnish.

@dridi
Copy link
Member

dridi commented Aug 21, 2018

I'm OK with the README changes, not the rest.

@dridi
Copy link
Member

dridi commented Aug 21, 2018

Maybe VC should not try to install builtin.vcl and example.vcl into doc in the first place?

I think we shouldn't mess with anything that comes from make install at packaging time.

@dmatetelki
Copy link
Contributor Author

Right now we install default.vcl and builtin.vcl twice, that's confusing at least.

default.vcl and example.vcl are identical, one builtin.vcl is a commented out version of the other.

deb:

/etc/varnish/default.vcl
/usr/share/doc/varnish/example.vcl
/usr/share/doc/varnish/builtin.vcl
/usr/share/doc/varnish/builtin.vcl.gz

rpm:

/etc/varnish/default.vcl
/usr/share/doc/varnish-4.1.10(!)/html, LICENSE, changes.html
/usr/share/doc/varnish/example.vcl
/usr/share/doc/varnish/builtin.vcl

@dridi dridi mentioned this pull request Aug 22, 2018
Open
@dridi
Copy link
Member

dridi commented Aug 22, 2018

default.vcl is a pure product of our packaging, but builtin.vcl comes from varnish-cache itself. example.vcl also comes from varnish-cache so we shouldn't touch them.

The fact that we maintain a default.vcl that is a copy of example.vcl is in my opinion something we should kill at some point in the weekly branch and keep maintaining for the 6.x series. Much like we simplified our packaging by no longer generating a secret and letting varnishd generate one, I'd like to replace the -f /etc/varnish/default option with -b localhost:8080 and let users decide how they operate their VCL. For reference: #106.

The builtin.vcl.gz file in the deb packages is something I would definitely remove, I have no idea where it comes from.

The fact that our RPMs ship the web documentation in /usr/share/doc/varnish-4.1.10/html instead of /usr/share/doc/varnish/html is simply a packaging policy from el6 and el7. With the same sources on Fedora, we'd get rid of the version in the directory name and I expect the same from el8 once released.

@dmatetelki
Copy link
Contributor Author

Sorry, the deb status is:

/etc/varnish/default.vcl
/usr/share/doc/varnish/example.vcl
/usr/share/doc/varnish/builtin.vcl
/usr/share/doc/varnish/examples/builtin.vcl.gz

Then all the changes what this PR will do is adding the README* files and removing the varnish.examples file causing /usr/share/doc/varnish/examples/builtin.vcl.gz

dridi
dridi reviewed Aug 22, 2018
View reviewed changes
redhat/varnish.spec Outdated
@@ -127,6 +127,7 @@ rm -rf %{buildroot}
%{_unitdir}/*
%exclude %{_datadir}/%{name}/vmodtool*
%exclude %{_datadir}/%{name}/vsctool*
README*
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about this?

I think it's %doc README*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, thanks.

@dmatetelki
Doc fixes
94ff840 
READMEs:
 - Installing again the once-installed README.rst and README.packaging

VCLs:
- A non-commented version of builtin.vcl is not installed into
/usr/share/doc/varnish/examples anymore.
@dridi dridi merged commit 8a6b351 into weekly Aug 23, 2018
@dridi dridi deleted the vc_weekly_docfix branch August 23, 2018 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dridi dridi dridi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dmatetelki @dridi