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

feat: poc octo run list #638

Open
wants to merge 49 commits into
base: master
Choose a base branch
from

Conversation

GustavEikaas
Copy link

@GustavEikaas GustavEikaas commented Oct 15, 2024

Important

This is a draft PR
Im making this draft PR so I can get reviews/suggestions during development
I will fill out the PR template when the PR is ready for review

Resolves: #149

Describe what this PR does / why we need it

First PR in a series of PRs to add support for github actions

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

Checklist

  • Passing tests and linting standards
  • Documentation updates in README.md and doc/octo.txt

Feature checklist

  • Buffer
    • Expand/collapse node
    • Highlights
    • Keybindings
      • Refresh
  • Workflows
    • Support multiple jobs
  • Workflow job steps
    • Logs
    • Status
    • ##[group]
    • [command]

Remaining

  • Customizable keymappings
  • Customizable icons
  • Update readme
  • Cleanup code
  • Customizable refresh interval
  • Look into using graphql in favor of gh cli commands (not sure if strictly necessary but conforms to the repo standard)
  • Use telescope pickers from the repo
  • Understand the writers.lua file and extend it
  • Use existing octo window creator

@wd60622
Copy link
Collaborator

wd60622 commented Oct 17, 2024

Using graphql is not a big deal. Just use whatever endpoint that works. Think there are a few non graphql commands used.

@GustavEikaas
Copy link
Author

Ah okay, functionality wise, do you have any feedback?

@wd60622
Copy link
Collaborator

wd60622 commented Oct 17, 2024

Ah okay, functionality wise, do you have any feedback?

I will give it a try when I can!

@wd60622 wd60622 added the enhancement New feature or request label Oct 17, 2024
@wd60622
Copy link
Collaborator

wd60622 commented Oct 18, 2024

Think it looks good. Would you want to make use of the pickers instead of the new, custom buffer? I have been using telescope recently. What do you use?

Some items:

  • What do you expect the callback to be upon selection an item? We can work something in like Abitrary callbacks for pickers #642 in order to support more in the future.
  • Would there be a preview or buffer to show each item? What would be populated there? Maybe a buffer doesn't make sense here. What are your thoughts?

lua/octo/workflow_runs.lua Outdated Show resolved Hide resolved
@GustavEikaas
Copy link
Author

image
@wd60622 Is this what you meant, kinda like the PR and issue flow?

@GustavEikaas
Copy link
Author

You still want there to be a buffer when you press <CR> on one of the items in the picker. AKA same flow as pull request?

@pwntester
Copy link
Owner

Thanks for the PR @GustavEikaas!

I like the idea of showing the status of the run in the preview buffer and perhaps show the logs of the run upon opening the item with <CR>. The problem I see is that a run may contain multiple jobs and each job will have its own log. Should we present them sequentially? thoughts?

@GustavEikaas
Copy link
Author

IIRC the sequential jobs is already supported but yes I was thinking the same.
For the logs of every step we should probably lazy load and use fold or something similiar. Some steps have very long log outputs

@GustavEikaas
Copy link
Author

@pwntester
Here is an image of what it looks like with multiple jobs
image

@GustavEikaas
Copy link
Author

Doesnt seem like there is a good structured output for workflow logs. Will probably be tricky to have the same UI as github web.
Only way I see right now is query the workflow logs and using regex to match the lines to the corresponding steps

@pwntester
Copy link
Owner

Nice, perhaps we can do a nested telescope selection where the use first chooses the run and then the job, that should get us a shorter log to show to the user and it should easier to print since we dont need to care about job's dependencies, just sequential steps

@wd60622
Copy link
Collaborator

wd60622 commented Jan 15, 2025

No worries. The experience doesn't have to be optimal. I just a working solution for finished logs would be fine. No need to aim for streaming. As you mentioned, looking at failed jobs is likely the most common usecase.

Let's try to get this merged in and people can try.

For me, it still doesn't allow me to open. I am getting that same traceback.

What is your current bandwidth for this PR?

@GustavEikaas
Copy link
Author

If the goal is to prioritize looking at failed jobs I can probably finish the PR this week. Ill look closer into your issue after work. Looks like os.tmpname fails to generate a unique name for you. weird...

@wd60622
Copy link
Collaborator

wd60622 commented Jan 15, 2025

I am on WSL. Not sure if that is related. I use tmp files for other programs no worries.

@GustavEikaas
Copy link
Author

@wd60622 After looking closer at the code im still thinking it fails to generate a unique tmpname which is pretty weird. I'm also using WSL but no issue for me.. Anyways I updated the code to handle the case of generating a unique tmpname. Would you mind testing again. Other than that I think the code is pretty much done, just a bunch of CR fixes remaining presumably. Feel free to review and comment the code

@GustavEikaas GustavEikaas marked this pull request as ready for review January 15, 2025 18:06
@wd60622
Copy link
Collaborator

wd60622 commented Jan 15, 2025

I've just updated my branch and still not working :( I will have to investigate further. Same traceback as before.

@GustavEikaas
Copy link
Author

Hmm if youre getting exactly the same traceback as before I think it means the plugin isnt updated with the latest commit. Could you try removing it and installing it again from the same branch? Im pretty sure I have ran into the same issue with lazy before, Update doesnt always work

@wd60622
Copy link
Collaborator

wd60622 commented Jan 15, 2025

This is my configuration. It should be using the latest version of your branch :

https://github.com/wd60622/dotfiles/blob/9b9dfdb2268a34f9c699c46fe89659ce6bdebfc0/nvim/lua/plugins/version-control.lua#L204-L204

@GustavEikaas
Copy link
Author

Does it reference the same tmpname in the stacktrace every time?

@wd60622
Copy link
Collaborator

wd60622 commented Jan 15, 2025

I was mistaken. This is a different traceback. Here are the results of three different attempts to open:

E5108: Error executing lua: ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:281: Failed to create a unique temporary file name after 5
0 attempts.
stack traceback:
        [C]: in function 'error'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:281: in function 'create_unique_tmpname'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:287: in function 'write_and_unzip_file'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:342: in function 'get_logs'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:402: in function 'cb'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:623: in function <...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:
619>
E5108: Error executing lua: ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:281: Failed to create a unique temporary file name after 5
0 attempts.
stack traceback:
        [C]: in function 'error'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:281: in function 'create_unique_tmpname'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:287: in function 'write_and_unzip_file'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:342: in function 'get_logs'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:402: in function 'cb'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:623: in function <...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:
619>
E5108: Error executing lua: ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:281: Failed to create a unique temporary file name after 5
0 attempts.
stack traceback:
        [C]: in function 'error'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:281: in function 'create_unique_tmpname'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:287: in function 'write_and_unzip_file'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:342: in function 'get_logs'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:402: in function 'cb'
        ...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:623: in function <...tHub/neovim-plugins/octo.nvim/lua/octo/workflow_runs.lua:
619>

@GustavEikaas
Copy link
Author

GustavEikaas commented Jan 15, 2025

@wd60622 That is crazy, here is the code. Looks a lot to me like your tmp folder is filled with an insane amount of files or there is something wrong with os.tmpname implementation? I guess I could try a different approach than os.tmpname but are we sure this is not related to your setup/environment?

local function create_unique_tmpname()
  local depth = 50
  local attempts = 0
  local tmpname

  while attempts < depth do
    tmpname = os.tmpname()
    local file = io.open(tmpname, "r")

    if not file then
      return tmpname
    else
      file:close()
    end

    attempts = attempts + 1
  end

  error("Failed to create a unique temporary file name after " .. depth .. " attempts.")
end

@wd60622
Copy link
Collaborator

wd60622 commented Jan 16, 2025

Yeah, weird. Thanks for sharing the snippet. Let me isolate it and see what happens

@wd60622
Copy link
Collaborator

wd60622 commented Jan 16, 2025

The files are being created in the /tmp folder. I see them after running the function isolated.

The file object looks like this: <userdata 1>

EDIT: I had some success with this:

local function create_unique_tmpname(opts)
  local depth = opts.depth or 50
  local attempts = 0
  local tmpname

  while attempts < depth do
    tmpname = os.tmpname()
    local file = io.open(tmpname, "w")

    if file then
      file:close()
      return tmpname
    end

    attempts = attempts + 1
  end

  error("Failed to create a unique temporary file name after " .. depth .. " attempts.")
end

local filename = create_unique_tmpname { depth = 2 }

@GustavEikaas
Copy link
Author

GustavEikaas commented Jan 16, 2025

hmm problem with your edited snippet is im preparing a new directory for unzip to unzip into. If I write to that file it will exist and thus cannot be overwritten. I need the function to generate a unique name for a file that does not yet exist. Whats weird however is how writing the file makes the problem go away for you. Strange...

@wd60622
Copy link
Collaborator

wd60622 commented Jan 16, 2025

It seems to create the file on read. Is there an directory exists function that could be used instead? Maybe in plenary

@GustavEikaas
Copy link
Author

If i put this in my init.lua function in throws an error when i start neovim. This is because the file does not exist and thus the filehandle is nil. But the following code seems to behave different for you and create the file?

local tmpname = os.tmpname()
local file = io.open(tmpname, "r")
file:close();
print(tmpname)

I guess i could replace the code with, expecting success == false
local tmpname = os.tmpname()
local success = pcall(vim.fn.readfile, tmpname)

@GustavEikaas
Copy link
Author

A lot of testing for you but maybe the latest one works? where i use vim.fn.readfile instead to check if file exists?

@GustavEikaas
Copy link
Author

Finally found it. Its a POSIX quirk. From the lua docs for os.tmpname

In POSIX systems, this function also creates a file with that name, to avoid security risks. (Someone else might create the file with wrong permissions in the time between getting the name and creating the file.) You still have to open the file to use it and to remove it (even if you do not use it).

@GustavEikaas
Copy link
Author

GustavEikaas commented Jan 16, 2025

Latest commit works fine on arch (btw), and windows. Should work just fine for you now @wd60622.
What weird behaviour for os.tmpname....

@wd60622
Copy link
Collaborator

wd60622 commented Jan 16, 2025

Thanks for the updates. will give it a try when I can

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request - GitHub actions
3 participants