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

Pass full command-line and use as transaction title #1824

Closed
wants to merge 2 commits into from
Closed

Pass full command-line and use as transaction title #1824

wants to merge 2 commits into from

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented May 1, 2019

In the app, rebuild the exact command-line that the client used and pass
that to the daemon to be used as the transaction title. Especially in
transactions like UpdateDeployment(), we can avoid reverse-engineering
what the original command used was.

This will be used by the upcoming history feature to record the
command-line used in the journal.

Split out of #1813.

@jlebon jlebon mentioned this pull request May 2, 2019
Closed
@jlebon
Copy link
Member Author

jlebon commented May 2, 2019

First three there should be pretty straight-forward; split those out in #1825.

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably aaccd48) made this pull request unmergeable. Please resolve the merge conflicts.

jlebon added 2 commits May 3, 2019 15:19
@jlebon
libpriv/util: Expand set of safe shell chars
1e12490 
Expand the set to include '/' and '=' since those don't need escaping in
shells.
@jlebon
Copy used command-line and set as transaction title
77302dc 
In the app, rebuild the exact command-line that the client used and pass
that to the daemon to be used as the transaction title. Especially in
transactions like `UpdateDeployment()`, we can avoid reverse-engineering
what the original command used was.

This will be used by the upcoming history feature to record the
command-line used in the journal.
@jlebon jlebon marked this pull request as ready for review May 3, 2019 19:20
@jlebon
Copy link
Member Author

jlebon commented May 3, 2019

OK, this one is ready for review!

@jlebon
Copy link
Member Author

jlebon commented May 3, 2019

bot, retest this please

@rfairley
Copy link
Contributor

rfairley commented May 6, 2019

Not sure what's causing this error - giving it another retest!

Installing       : libffi-devel-3.1-18.fc29.x86_64                     89/177 
  Running scriptlet: libffi-devel-3.1-18.fc29.x86_64                     89/177time="2019-05-03T19:39:23Z" level=error msg="error getting events from daemon: unexpected EOF"

@rfairley
Copy link
Contributor

rfairley commented May 6, 2019

bot, retest this please

@jlebon
Copy link
Member Author

jlebon commented May 8, 2019

Whee, all green now! ✔️

@rfairley
Copy link
Contributor

rfairley commented May 8, 2019

Scanned over, and this LGTM!

@rh-atomic-bot r+ 77302dc

@rh-atomic-bot
Copy link

⌛ Testing commit 77302dc with merge 3952cf4...

rh-atomic-bot pushed a commit that referenced this pull request May 8, 2019
@jlebon @rh-atomic-bot
libpriv/util: Expand set of safe shell chars
59381a4 
Expand the set to include '/' and '=' since those don't need escaping in
shells.

Closes: #1824
Approved by: rfairley
rh-atomic-bot pushed a commit that referenced this pull request May 8, 2019
@jlebon @rh-atomic-bot
Copy used command-line and set as transaction title
3952cf4 
In the app, rebuild the exact command-line that the client used and pass
that to the daemon to be used as the transaction title. Especially in
transactions like `UpdateDeployment()`, we can avoid reverse-engineering
what the original command used was.

This will be used by the upcoming history feature to record the
command-line used in the journal.

Closes: #1824
Approved by: rfairley
@rfairley
Copy link
Contributor

rfairley commented May 8, 2019

@rh-atomic-bot retry

@rh-atomic-bot
Copy link

⌛ Testing commit 77302dc with merge 0858e6f...

rh-atomic-bot pushed a commit that referenced this pull request May 8, 2019
@jlebon @rh-atomic-bot
libpriv/util: Expand set of safe shell chars
bccb70f 
Expand the set to include '/' and '=' since those don't need escaping in
shells.

Closes: #1824
Approved by: rfairley
rh-atomic-bot pushed a commit that referenced this pull request May 8, 2019
@jlebon @rh-atomic-bot
Copy used command-line and set as transaction title
0858e6f 
In the app, rebuild the exact command-line that the client used and pass
that to the daemon to be used as the transaction title. Especially in
transactions like `UpdateDeployment()`, we can avoid reverse-engineering
what the original command used was.

This will be used by the upcoming history feature to record the
command-line used in the journal.

Closes: #1824
Approved by: rfairley
@jlebon
Copy link
Member Author

jlebon commented May 8, 2019 

Trying to pull repository registry.fedoraproject.org/fedora ... 
sha256:c95f9b2a2d5ee861904b9af2c98a3251faf4ec099f625cfee38ffe6a349fc980: Pulling from registry.fedoraproject.org/fedora
8dba660c242f: Pulling fs layer
8dba660c242f: Verifying Checksum
8dba660c242f: Download complete
/usr/bin/docker-current: unexpected EOF.
See '/usr/bin/docker-current run --help'.
### EXITED WITH CODE 125 AFTER 8s

😕

@rh-atomic-bot retry

@cgwalters
Copy link
Member

I'm OK with this but...there won't be a (useful) command line for gnome-software for example. I guess though a lot of other automation agents are probably just going to end up executing the binary.

@rh-atomic-bot
Copy link

⌛ Testing commit 77302dc with merge 4c99cc7...

rh-atomic-bot pushed a commit that referenced this pull request May 8, 2019
@jlebon @rh-atomic-bot
Copy used command-line and set as transaction title
4c99cc7 
In the app, rebuild the exact command-line that the client used and pass
that to the daemon to be used as the transaction title. Especially in
transactions like `UpdateDeployment()`, we can avoid reverse-engineering
what the original command used was.

This will be used by the upcoming history feature to record the
command-line used in the journal.

Closes: #1824
Approved by: rfairley
@jlebon
Copy link
Member Author

jlebon commented May 8, 2019

I'm OK with this but...there won't be a (useful) command line for gnome-software for example

Yeah, I left the fallback heuristics in place in case some other client triggered us. All this is just extra sugar, not essential. My goal specifically here is to make it show up in the ex history entries. For GNOME Software, I think we could improve the situation by logging the client id and heuristic string into the history entry. E.g. changing the journal field to DEPLOYMENT_OPERATION or something.

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: rfairley
Pushing 4c99cc7 to master...

@jlebon jlebon deleted the pr/cmdline branch April 23, 2023 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jlebon @rh-atomic-bot @rfairley @cgwalters