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

time: no Parse format for GNU style RFC3339 #25937

Closed
stub42 opened this issue Jun 18, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@stub42
Copy link

commented Jun 18, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.10.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Ubuntu 18.04, amd64

What did you do?

Attempted to time.Parse the result of 'date --utc --rfc-3339=ns':

time.Parse(time.RFC3339Nano, "2018-06-18 10:19:31.800140702+00:00")
package main

import (
	"fmt"
	"os/exec"
	"strings"
	"time"
)

func main() {
	raw, err := exec.Command("date", "--utc", "--rfc-3339=ns").Output()
	if err != nil {
		panic(fmt.Sprintf("failed to run date: %v", err))
	}
	from_ubuntu := strings.TrimSpace(string(raw))
	t, err := time.Parse(time.RFC3339Nano, from_ubuntu)
	if err != nil {
		fmt.Printf("Unparsable: %q\n", string(from_ubuntu))
	} else {
		fmt.Printf("Parsed: %v\n", t)
	}
}

What did you expect to see?

Parsing to succeed

What did you see instead?

Failure, due to an old disagreement about the RFC: http://lists.gnu.org/archive/html/bug-coreutils/2006-05/msg00019.html

GNU coreutils team maintain whitespace is allowed, but the Go library requires the 'T' separator between the date and time components of the string representation.

Options to allow interoperability between GNU style and Go style RFC-3339 complient strings would be to either relax the Go parsing, or provide alternative predefined styles in the time package (time.RFC3339GNU and time.RFC3339NanoGNU, or time.GNU3339 and time.GNU3339Nano, or similar)

@josharian josharian changed the title No time.Parse format for GNU style RFC3339 time: no Parse format for GNU style RFC3339 Jun 18, 2018

@agnivade

This comment has been minimized.

Copy link
Member

commented Jun 18, 2018

/cc @rsc

@agnivade agnivade added this to the Unplanned milestone Jun 18, 2018

@ulikunitz

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2018

While there is a note in RFC3339 saying that applications may choose to replace the T with a space, this is not what the formal ABNF specification there supports. The underlying ISO 8601 standards mandates the use of the T. Since any application choosing to support space can easily define their own constants, I don't think new constants in time are needed.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2018

I don't see a need to change anything here. We appear to follow the RFC. It's easy to adapt.

@crawford

This comment has been minimized.

Copy link

commented Oct 12, 2018

Taken directly from the RFC:

NOTE: ISO 8601 defines date and time separated by "T".
Applications using this syntax may choose, for the sake of
readability, to specify a full-date and full-time separated by
(say) a space character.

Yes, the ABNF doesn't included characters other than T, but there is a second note which makes it clear that the ABNF is not exhaustive:

NOTE: Per [ABNF] and ISO8601, the "T" and "Z" characters in this
syntax may alternatively be lower case "t" or "z" respectively.

As such, I would argue that the time library doesn't fully conform to RFC3339.

crawford added a commit to crawford/installer that referenced this issue Oct 12, 2018

asset/ignition: add event on bootstrap completion
In order to allow the installer to eventually track the installation
progress, this adds a reporting service which inserts an event when the
bootstrap process has completed. This event can be used to determine
when it is safe to remove the bootstrap node, since it signifies that
the bootstrap control plane has pivoted to the scheduled control plane
and that all of the OpenShift manifests have been inserted.

In the creation of the event, `date` is invoked and the output piped
into `tr`. This was necessary because Golang requires the `T` separator
in the time package [1] but GNU's `date` uses a space separator. ISO8601
was also tried, but Golang doesn't seem to parse that correctly either
(`date` uses "+0000" to specify timezone but Golang expects "+00:00").

[1]: golang/go#25937

crawford added a commit to crawford/installer that referenced this issue Oct 12, 2018

asset/ignition: add event on bootstrap completion
In order to allow the installer to eventually track the installation
progress, this adds a reporting service which inserts an event when the
bootstrap process has completed. This event can be used to determine
when it is safe to remove the bootstrap node, since it signifies that
the bootstrap control plane has pivoted to the scheduled control plane
and that all of the OpenShift manifests have been inserted.

In the creation of the event, `date` is invoked and the output piped
into `tr`. This was necessary because Golang requires the `T` separator
in the time package [1] but GNU's `date` uses a space separator. ISO8601
was also tried, but Golang doesn't seem to parse that correctly either
(`date` uses "+0000" to specify timezone but Golang expects "+00:00").

[1]: golang/go#25937
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

@crawford This issue is closed, and we don't track comments on closed issues. You can write formats yourself to do whatever you need.

crawford added a commit to crawford/installer that referenced this issue Oct 15, 2018

asset/ignition: add event on bootstrap completion
In order to allow the installer to eventually track the installation
progress, this adds a reporting service which inserts an event when the
bootstrap process has completed. This event can be used to determine
when it is safe to remove the bootstrap node, since it signifies that
the bootstrap control plane has pivoted to the scheduled control plane
and that all of the OpenShift manifests have been inserted.

This no longer enables `tectonic.service` because `progress.service` is
enabled and lists both `tectonic.service` and `bootkube.service` as
dependencies. systemd will start all three services as a result.

In an ideal world, this would make use of sd-notify to signal the
completion of prerequisites but this doesn't work for two reasons.
First, runc doesn't support passing the extra file descriptor used by
sd-notify [1]. Second, systemd doesn't support retrying activations when
a dependency has failed [2]. This change makes use of "breadcrumb" files
to signal completion as a workaround.

In the creation of the event, `date` is invoked and passed a custom
format. This was necessary because Golang requires the `T` separator in
the time package [3] but GNU's `date` uses a space separator. ISO8601
was also tried, but Golang doesn't seem to parse that correctly either
(`date` uses "+0000" to specify timezone but Golang expects "+00:00").

[1]: opencontainers/runc#1807
[2]: systemd/systemd#1312
[3]: golang/go#25937

crawford added a commit to crawford/installer that referenced this issue Oct 15, 2018

asset/ignition: add event on bootstrap completion
In order to allow the installer to eventually track the installation
progress, this adds a reporting service which inserts an event when the
bootstrap process has completed. This event can be used to determine
when it is safe to remove the bootstrap node, since it signifies that
the bootstrap control plane has pivoted to the scheduled control plane
and that all of the OpenShift manifests have been inserted.

This no longer enables `tectonic.service` because `progress.service` is
enabled and lists both `tectonic.service` and `bootkube.service` as
dependencies. systemd will start all three services as a result.

In an ideal world, this would make use of sd-notify to signal the
completion of prerequisites but this doesn't work for two reasons.
First, runc doesn't support passing the extra file descriptor used by
sd-notify [1]. Second, systemd doesn't support retrying activations when
a dependency has failed [2]. This change makes use of "breadcrumb" files
to signal completion as a workaround.

In the creation of the event, `date` is invoked and passed a custom
format. This was necessary because Golang requires the `T` separator in
the time package [3] but GNU's `date` uses a space separator. ISO8601
was also tried, but Golang doesn't seem to parse that correctly either
(`date` uses "+0000" to specify timezone but Golang expects "+00:00").

[1]: opencontainers/runc#1807
[2]: systemd/systemd#1312
[3]: golang/go#25937

crawford added a commit to crawford/installer that referenced this issue Oct 15, 2018

asset/ignition: add event on bootstrap completion
In order to allow the installer to eventually track the installation
progress, this adds a reporting service which inserts an event when the
bootstrap process has completed. This event can be used to determine
when it is safe to remove the bootstrap node, since it signifies that
the bootstrap control plane has pivoted to the scheduled control plane
and that all of the OpenShift manifests have been inserted.

This no longer enables `tectonic.service` because `progress.service` is
enabled and lists both `tectonic.service` and `bootkube.service` as
dependencies. systemd will start all three services as a result.

In an ideal world, this would make use of sd-notify to signal the
completion of prerequisites but this doesn't work for two reasons.
First, runc doesn't support passing the extra file descriptor used by
sd-notify [1]. Second, systemd doesn't support retrying activations when
a dependency has failed [2]. This change makes use of "breadcrumb" files
to signal completion as a workaround.

In the creation of the event, `date` is invoked and passed a custom
format. This was necessary because Golang requires the `T` separator in
the time package [3] but GNU's `date` uses a space separator. ISO8601
was also tried, but Golang doesn't seem to parse that correctly either
(`date` uses "+0000" to specify timezone but Golang expects "+00:00").

[1]: opencontainers/runc#1807
[2]: systemd/systemd#1312
[3]: golang/go#25937

crawford added a commit to crawford/installer that referenced this issue Oct 15, 2018

asset/ignition: add event on bootstrap completion
In order to allow the installer to eventually track the installation
progress, this adds a reporting service which inserts an event when the
bootstrap process has completed. This event can be used to determine
when it is safe to remove the bootstrap node, since it signifies that
the bootstrap control plane has pivoted to the scheduled control plane
and that all of the OpenShift manifests have been inserted.

This no longer enables `tectonic.service` because `progress.service` is
enabled and lists both `tectonic.service` and `bootkube.service` as
dependencies. systemd will start all three services as a result.

In an ideal world, this would make use of sd-notify to signal the
completion of prerequisites but this doesn't work for two reasons.
First, runc doesn't support passing the extra file descriptor used by
sd-notify [1]. Second, systemd doesn't support retrying activations when
a dependency has failed [2]. This change makes use of "breadcrumb" files
to signal completion as a workaround.

In the creation of the event, `date` is invoked and passed a custom
format. This was necessary because Golang requires the `T` separator in
the time package [3] but GNU's `date` uses a space separator. ISO8601
was also tried, but Golang doesn't seem to parse that correctly either
(`date` uses "+0000" to specify timezone but Golang expects "+00:00").

[1]: opencontainers/runc#1807
[2]: systemd/systemd#1312
[3]: golang/go#25937
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.