Update http (not s) refs to IETF URI to use https

[machawk1]

There are a few references throughout the codebase/tests to http://tools.ietf.org/html/rfc7089, which results in a 404 when dereferenced. This PR uses the same URI but with https as the scheme, which resolves.

Perhaps trivial but also critical to provide the context where it is used.

[machawk1]

I noticed a strange issue here that is causing the tests to fail. In the GitHub Action, the sample CDXJ file are pulled from the master branch. For these tests to be representative, especially if modified or new ones added, they ought to use the samples in the branch from which the PR is contributing.

This PR (#739) is a good opportunity to fix this issue in testing.

[machawk1]

Based on https://raw.githubusercontent.com/oduwsdl/ipwb/master/.github/workflows/test.yml , can you suggest how we go about using the branch's data as the basis for the tests? Is it even a good idea to do this, @ibnesayeed?

[machawk1]

Something like:

- uses: actions/checkout@v2
  with:
    ref: ${{ github.event.pull_request.head.sha }}

...might work but would only be applicable to PRs. Ideally, changes should be made to master directly anyway, so perhaps this will help to enforce that.

[machawk1]

Taking a step back, this is not an issue with the GitHub Action but rather, the absolute URI is embedded in the test code, e.g.,

ipwb/tests/test_backends.py

Lines 21 to 25 in 6b74548

	def test_https():
	assert get_web_archive_index(
	'https://raw.githubusercontent.com/oduwsdl/ipwb/master/samples/' +
	'indexes/salam-home.cdxj'
	).startswith('!context ["http://tools.ietf.org/html/rfc7089"]')

...and this is probably fine here, as it is testing the ability to pull in a remote index and its validity in the first line.

[ibnesayeed]

There are a few references throughout the codebase/tests to http://tools.ietf.org/html/rfc7089, which results in a 404 when dereferenced. This PR uses the same URI but with https as the scheme, which resolves.

This was likely an transient issue. I did check earlier that curling HTTP URI returned 404 response while accessing the same from the browser performed redirects. Now, the redirection is working from curl as well.

[machawk1]

@ibnesayeed I encountered it intermittently as well but during this time, the HTTPS URI consistently worked. Any issue w/ merging this?

[ibnesayeed]

I am not sure if this should be here because this workflow is executed on push as well, for which I do not know what would be the PR head.

[machawk1]

Agreed! This was added during the discovery phase before I realized that the absolute reference to the URI using the master branch was embedded in the test case. I will remove this change to test.yml.

[machawk1]

Removed the change to test.yml in f50d06f (below)

[ibnesayeed]

@ibnesayeed I encountered it intermittently as well but during this time, the HTTPS URI consistently worked. Any issue w/ merging this?

I am okay with URIs changed to HTTPS, but not sure about other changes.

[ibnesayeed]

Based on https://raw.githubusercontent.com/oduwsdl/ipwb/master/.github/workflows/test.yml , can you suggest how we go about using the branch's data as the basis for the tests? Is it even a good idea to do this, @ibnesayeed?

The documentation suggests that it should be the default behavior:

    # The branch, tag or SHA to checkout. When checking out the repository that
    # triggered a workflow, this defaults to the reference or SHA for that event.
    # Otherwise, uses the default branch.
    ref: ''

[machawk1]

The addition sanity check of echo'ing the branch being used with the removal of explicitly changing branches in f50d06f will validate the default behavior.

[ibnesayeed]

Do we need this step?

[machawk1]

That is the aforementioned sanity check. Not necessary but useful in the currently running GitHub action to validate the "default behavior". If the default behavior is, in fact, to use the PR commit instead of the main branch, I will remove it.

[ibnesayeed]

The addition sanity check of echo'ing the branch being used with the removal of explicitly changing branches in f50d06f will validate the default behavior.

I am not sure if we need to verify a well-tested and well-used aspect of external code.

[machawk1]

I have removed the sanity check in afa7405, @ibnesayeed. Any other suggestions before we merge this?

[ibnesayeed]

LGTM!