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

labelled outputs #121

Merged
merged 2 commits into from
Jan 2, 2025
Merged

labelled outputs #121

merged 2 commits into from
Jan 2, 2025

Conversation

burnout87
Copy link
Contributor

No description provided.

@burnout87
Copy link
Contributor Author

burnout87 commented Jan 2, 2025 •
edited
Loading 

remote: Invalid username or password.
fatal: Authentication failed for 'https://github.com/oda-hub/oda-hub.github.io/'

@volodymyrss I guess this secret needs to be updated ?

@volodymyrss
Copy link
Member

Probably, but I dont remember why it tries to publish before merge. Could you check that it does not change prod site in PR?

@burnout87
Copy link
Contributor Author

Probably, but I dont remember why it tries to publish before merge. Could you check that it does not change prod site in PR?

The prod site is not affected by this PR, is that what you asked?

@volodymyrss
Copy link
Member

Probably, but I dont remember why it tries to publish before merge. Could you check that it does not change prod site in PR?

The prod site is not affected by this PR, is that what you asked?

I mean that the job that fails, the one you mentioned, is because it can not publish production documentation site, right? It should not attempt to do it before the merge.

@dsavchenko
Copy link
Member

dsavchenko commented Jan 2, 2025 •
edited
Loading

Looking at the workflow file, I don't see any distinction between PR and merge to master. It publishes in both cases.
Having PR preview is useful, though, so we may use something like in https://github.com/esg-epfl-apc/astrojsvis/blob/main/.github/workflows/preview-pr.yml

@burnout87
Copy link
Contributor Author

Looking at the workflow file, I don't see any distinction between PR and merge to master. It publishes in both cases.

Indeed, perhaps the publishing part should be moved to a separate workflow that is triggered only when merging on master

@ferrigno
Copy link
Contributor

ferrigno commented Jan 2, 2025

at the end of lines 149-151, there a full stop (.) at the end of each line, Is it relevant? If not, I suggest to remove otherwise it should be explained.

ferrigno
ferrigno approved these changes Jan 2, 2025
View reviewed changes
Copy link
Contributor

@ferrigno ferrigno left a comment

Choose a reason for hiding this comment

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

See my comment on the point at the end of code lines 149-151 to be removed or explained.

@burnout87
Copy link
Contributor Author

burnout87 commented Jan 2, 2025 •
edited
Loading

at the end of lines 149-151, there a full stop (.) at the end of each line, Is it relevant? If not, I suggest to remove otherwise it should be explained.

It is not really, the execution works with and without ... but I will remove it for consistency

@burnout87 burnout87 merged commit 55ee90d into master Jan 2, 2025
1 of 2 checks passed
@burnout87 burnout87 linked an issue Jan 2, 2025 that may be closed by this pull request
add how to label output #120
Closed
@dsavchenko dsavchenko mentioned this pull request Jan 2, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsavchenko dsavchenko dsavchenko approved these changes

@volodymyrss volodymyrss volodymyrss approved these changes

@ferrigno ferrigno ferrigno approved these changes

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

Successfully merging this pull request may close these issues.

add how to label output
4 participants
@burnout87 @volodymyrss @dsavchenko @ferrigno