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

Check disabled workflow when rerun jobs #26535

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -3503,6 +3503,7 @@ workflow.disable = Disable Workflow
workflow.disable_success = Workflow '%s' disabled successfully.
workflow.enable = Enable Workflow
workflow.enable_success = Workflow '%s' enabled successfully.
workflow.disabled = Workflow is disabled.

need_approval_desc = Need approval to run workflows for fork pull request.

Expand Down
28 changes: 16 additions & 12 deletions routers/web/repo/actions/view.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,31 +259,35 @@ func ViewPost(ctx *context_module.Context) {
ctx.JSON(http.StatusOK, resp)
}

func RerunOne(ctx *context_module.Context) {
// Rerun will rerun jobs in the given run
// jobIndex = 0 means rerun all jobs
func Rerun(ctx *context_module.Context) {
runIndex := ctx.ParamsInt64("run")
jobIndex := ctx.ParamsInt64("job")

job, _ := getRunJobs(ctx, runIndex, jobIndex)
if ctx.Written() {
run, err := actions_model.GetRunByIndex(ctx, ctx.Repo.Repository.ID, runIndex)
if err != nil {
ctx.Error(http.StatusInternalServerError, err.Error())
return
}

if err := rerunJob(ctx, job); err != nil {
ctx.Error(http.StatusInternalServerError, err.Error())
// can not rerun job when workflow is disabled
cfgUnit := ctx.Repo.Repository.MustGetUnit(ctx, unit.TypeActions)
cfg := cfgUnit.ActionsConfig()
if cfg.IsWorkflowDisabled(run.WorkflowID) {
ctx.JSONError(ctx.Locale.Tr("actions.workflow.disabled"))
return
}

ctx.JSON(http.StatusOK, struct{}{})
}

func RerunAll(ctx *context_module.Context) {
runIndex := ctx.ParamsInt64("run")

_, jobs := getRunJobs(ctx, runIndex, 0)
job, jobs := getRunJobs(ctx, runIndex, jobIndex)
if ctx.Written() {
return
}

if jobIndex != 0 {
jobs = []*actions_model.ActionRunJob{job}
}

for _, j := range jobs {
if err := rerunJob(ctx, j); err != nil {
ctx.Error(http.StatusInternalServerError, err.Error())
Expand Down
4 changes: 2 additions & 2 deletions routers/web/web.go
Original file line number Diff line number Diff line change
Expand Up @@ -1211,14 +1211,14 @@ func registerRoutes(m *web.Route) {
m.Combo("").
Get(actions.View).
Post(web.Bind(actions.ViewRequest{}), actions.ViewPost)
m.Post("/rerun", reqRepoActionsWriter, actions.RerunOne)
m.Post("/rerun", reqRepoActionsWriter, actions.Rerun)
m.Get("/logs", actions.Logs)
})
m.Post("/cancel", reqRepoActionsWriter, actions.Cancel)
m.Post("/approve", reqRepoActionsWriter, actions.Approve)
m.Post("/artifacts", actions.ArtifactsView)
m.Get("/artifacts/{artifact_name}", actions.ArtifactsDownloadView)
m.Post("/rerun", reqRepoActionsWriter, actions.RerunAll)
m.Post("/rerun", reqRepoActionsWriter, actions.Rerun)
})
}, reqRepoActionsReader, actions.MustEnableActions)

Expand Down
2 changes: 2 additions & 0 deletions templates/base/head_script.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ If you are customizing Gitea, please do not change this file.
If you introduce mistakes in it, Gitea JavaScript code wouldn't run correctly.
*/}}
<script>
{{/* before our JS code gets loaded, use arrays to store errors, then the arrays will be switched to our error handler later */}}
window.addEventListener('error', function(e) {window._globalHandlerErrors=window._globalHandlerErrors||[]; window._globalHandlerErrors.push(e);});
window.addEventListener('unhandledrejection', function(e) {window._globalHandlerErrors=window._globalHandlerErrors||[]; window._globalHandlerErrors.push(e);});
window.config = {
appUrl: '{{AppUrl}}',
appSubUrl: '{{AppSubUrl}}',
Expand Down
10 changes: 9 additions & 1 deletion web_src/js/bootstrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ export function showGlobalErrorMessage(msg) {
* @param {ErrorEvent} e
*/
function processWindowErrorEvent(e) {
if (e.type === 'unhandledrejection') {
showGlobalErrorMessage(`JavaScript promise rejection: ${e.reason}. Open browser console to see more details.`);
return;
}
if (!e.error && e.lineno === 0 && e.colno === 0 && e.filename === '' && window.navigator.userAgent.includes('FxiOS/')) {
// At the moment, Firefox (iOS) (10x) has an engine bug. See https://github.com/go-gitea/gitea/issues/20240
// If a script inserts a newly created (and content changed) element into DOM, there will be a nonsense error event reporting: Script error: line 0, col 0.
Expand All @@ -30,6 +34,10 @@ function processWindowErrorEvent(e) {
}

function initGlobalErrorHandler() {
if (window._globalHandlerErrors?._inited) {
showGlobalErrorMessage(`The global error handler has been initialized, do not initialize it again`);
return;
}
if (!window.config) {
showGlobalErrorMessage(`Gitea JavaScript code couldn't run correctly, please check your custom templates`);
}
Expand All @@ -40,7 +48,7 @@ function initGlobalErrorHandler() {
processWindowErrorEvent(e);
}
// then, change _globalHandlerErrors to an object with push method, to process further error events directly
window._globalHandlerErrors = {'push': (e) => processWindowErrorEvent(e)};
window._globalHandlerErrors = {_inited: true, push: (e) => processWindowErrorEvent(e)};
}
Copy link
Member

Choose a reason for hiding this comment

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

@wxiaoguang could you port the Proxy implementation from #25940 here instead of extending this hack further? I think #25940 is not going to land anytime soon, so it's a good extract.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I have different opinion. I think this "array" is a right approach. The reasons are:

  1. The bug you met is not caused by the array, but by duplicate-init. I have added "inited" check in this PR, so you won't see any new problem.
  2. Array is also widely used by big company products, for example, Google Analytics also uses ga.push(), the same method.
  3. Using an object with "push" method is light than using a proxy object, and it doesn't need too much tricky code in the "set" method.

So, I would always prefer this approach.

Copy link
Member

Choose a reason for hiding this comment

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

Does your hack even run in JS strict mode? Try adding "use strict"; to the file 😆.

Copy link
Contributor

@wxiaoguang wxiaoguang Aug 25, 2023

Choose a reason for hiding this comment

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

It's not hack IMO, it strictly follows JS standard. And, why not run? I don't see any problem.

<script>
	window._testGlobal = [];
	window._testGlobal.push('foo');
</script>
<script src="{{AssetUrlPrefix}}/test.js"></script>
"use strict";

function test() {
  for (const e of window._testGlobal || []) {
    console.log(`test (pre): ${e}`);
  }
  window._testGlobal = {_inited: true, push: (e) => console.log(`test (after): ${e}`)};
}
test();

window._testGlobal.push('bar');

image

Copy link
Member

@silverwind silverwind Aug 25, 2023

Choose a reason for hiding this comment

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

I guess you are right and the problem I've been seeing may be because of that missing init. Still I personally prefer the Proxy method.

BTW, I would like our code to run in strict mode, but don't feel like littering every file with a "use strict". Maybe there are solutions for webpack to automatically inject it into every chunk like https://babeljs.io/docs/babel-plugin-transform-strict-mode.

Copy link
Member

Choose a reason for hiding this comment

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

According to evanw/esbuild#422 (comment) we could use the esbuild banner feature for it.


initGlobalErrorHandler();
15 changes: 2 additions & 13 deletions web_src/js/components/RepoActionView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<button class="ui basic small compact button red" @click="cancelRun()" v-else-if="run.canCancel">
{{ locale.cancel }}
</button>
<button class="ui basic small compact button gt-mr-0" @click="rerun()" v-else-if="run.canRerun">
<button class="ui basic small compact button gt-mr-0 link-action" :data-url="`${run.link}/rerun`" v-else-if="run.canRerun">
{{ locale.rerun_all }}
</button>
</div>
Expand All @@ -38,7 +38,7 @@
<span class="job-brief-name gt-mx-3 gt-ellipsis">{{ job.name }}</span>
</div>
<span class="job-brief-item-right">
<SvgIcon name="octicon-sync" role="button" :data-tooltip-content="locale.rerun" class="job-brief-rerun gt-mx-3" @click="rerunJob(index)" v-if="job.canRerun && onHoverRerunIndex === job.id"/>
<SvgIcon name="octicon-sync" role="button" :data-tooltip-content="locale.rerun" class="job-brief-rerun gt-mx-3 link-action" :data-url="`${run.link}/jobs/${index}/rerun`" v-if="job.canRerun && onHoverRerunIndex === job.id"/>
<span class="step-summary-duration">{{ job.duration }}</span>
</span>
</a>
Expand Down Expand Up @@ -264,17 +264,6 @@ const sfc = {
this.loadJob(); // try to load the data immediately instead of waiting for next timer interval
}
},
// rerun a job
async rerunJob(idx) {
const jobLink = `${this.run.link}/jobs/${idx}`;
await this.fetchPost(`${jobLink}/rerun`);
window.location.href = jobLink;
},
// rerun workflow
async rerun() {
await this.fetchPost(`${this.run.link}/rerun`);
window.location.href = this.run.link;
},
// cancel a run
cancelRun() {
this.fetchPost(`${this.run.link}/cancel`);
Expand Down
116 changes: 44 additions & 72 deletions web_src/js/features/common-global.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {handleGlobalEnterQuickSubmit} from './comp/QuickSubmit.js';
import {svg} from '../svg.js';
import {hideElem, showElem, toggleElem} from '../utils/dom.js';
import {htmlEscape} from 'escape-goat';
import {createTippy, showTemporaryTooltip} from '../modules/tippy.js';
import {showTemporaryTooltip} from '../modules/tippy.js';
import {confirmModal} from './comp/ConfirmModal.js';
import {showErrorToast} from '../modules/toast.js';

Expand Down Expand Up @@ -64,9 +64,9 @@ export function initGlobalButtonClickOnEnter() {
});
}

// doRedirect does real redirection to bypass the browser's limitations of "location"
// fetchActionDoRedirect does real redirection to bypass the browser's limitations of "location"
// more details are in the backend's fetch-redirect handler
function doRedirect(redirect) {
function fetchActionDoRedirect(redirect) {
const form = document.createElement('form');
const input = document.createElement('input');
form.method = 'post';
Expand All @@ -79,6 +79,33 @@ function doRedirect(redirect) {
form.submit();
}

async function fetchActionDoRequest(actionElem, url, opt) {
try {
const resp = await fetch(url, opt);
if (resp.status === 200) {
let {redirect} = await resp.json();
redirect = redirect || actionElem.getAttribute('data-redirect');
actionElem.classList.remove('dirty'); // remove the areYouSure check before reloading
if (redirect) {
fetchActionDoRedirect(redirect);
} else {
window.location.reload();
}
Copy link
Member

Choose a reason for hiding this comment

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

I know this is old code, but shouldn't we use 302 status code and then check resp.redirected and read location from resp.url? I think this is something for a future generic fetch handler.

Copy link
Contributor

@wxiaoguang wxiaoguang Aug 19, 2023

Choose a reason for hiding this comment

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

Nope IMO.

Fetch's redirect could only have 3 behaviors: follow, error, manual.

follow is the default behavior, it doesn't distinguish 200/302, etc, error just stops redirection.

If you would like to use "redirect": "manual" .... IIRC it doesn't work well (the details are too complex)

You could take a try, if it would succeed, feel free to propose a new solution.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that was implying manual option, but I guess it'll be hard to distinguish a "content redirect" from an "ui redirect", so I guess I'm fine with leaving it unchanged.

} else if (resp.status >= 400 && resp.status < 500) {
const data = await resp.json();
// the code was quite messy, sometimes the backend uses "err", sometimes it uses "error", and even "user_error"
// but at the moment, as a new approach, we only use "errorMessage" here, backend can use JSONError() to respond.
await showErrorToast(data.errorMessage || `server error: ${resp.status}`);
} else {
await showErrorToast(`server error: ${resp.status}`);
}
} catch (e) {
console.error('error when doRequest', e);
actionElem.classList.remove('is-loading', 'small-loading-icon');
await showErrorToast(i18n.network_error);
}
}

async function formFetchAction(e) {
if (!e.target.classList.contains('form-fetch-action')) return;

Expand Down Expand Up @@ -115,50 +142,7 @@ async function formFetchAction(e) {
reqOpt.body = formData;
}

let errorTippy;
const onError = (msg) => {
formEl.classList.remove('is-loading', 'small-loading-icon');
if (errorTippy) errorTippy.destroy();
// TODO: use a better toast UI instead of the tippy. If the form height is large, the tippy position is not good
errorTippy = createTippy(formEl, {
content: msg,
interactive: true,
showOnCreate: true,
hideOnClick: true,
role: 'alert',
theme: 'form-fetch-error',
trigger: 'manual',
arrow: false,
});
};

const doRequest = async () => {
try {
const resp = await fetch(reqUrl, reqOpt);
if (resp.status === 200) {
const {redirect} = await resp.json();
formEl.classList.remove('dirty'); // remove the areYouSure check before reloading
if (redirect) {
doRedirect(redirect);
} else {
window.location.reload();
}
} else if (resp.status >= 400 && resp.status < 500) {
const data = await resp.json();
// the code was quite messy, sometimes the backend uses "err", sometimes it uses "error", and even "user_error"
// but at the moment, as a new approach, we only use "errorMessage" here, backend can use JSONError() to respond.
onError(data.errorMessage || `server error: ${resp.status}`);
} else {
onError(`server error: ${resp.status}`);
}
} catch (e) {
console.error('error when doRequest', e);
onError(i18n.network_error);
}
};

// TODO: add "confirm" support like "link-action" in the future
await doRequest();
await fetchActionDoRequest(formEl, reqUrl, reqOpt);
}

export function initGlobalCommon() {
Expand Down Expand Up @@ -209,6 +193,7 @@ export function initGlobalCommon() {
$('.tabular.menu .item').tab();

document.addEventListener('submit', formFetchAction);
document.addEventListener('click', linkAction);
}

export function initGlobalDropzone() {
Expand Down Expand Up @@ -269,41 +254,29 @@ export function initGlobalDropzone() {
}

async function linkAction(e) {
e.preventDefault();

// A "link-action" can post AJAX request to its "data-url"
// Then the browser is redirected to: the "redirect" in response, or "data-redirect" attribute, or current URL by reloading.
// If the "link-action" has "data-modal-confirm" attribute, a confirm modal dialog will be shown before taking action.
const el = e.target.closest('.link-action');
if (!el) return;

const $this = $(this);
const redirect = $this.attr('data-redirect');

const doRequest = () => {
$this.prop('disabled', true);
$.post($this.attr('data-url'), {
_csrf: csrfToken
}).done((data) => {
if (data && data.redirect) {
window.location.href = data.redirect;
} else if (redirect) {
window.location.href = redirect;
} else {
window.location.reload();
}
}).always(() => {
$this.prop('disabled', false);
});
e.preventDefault();
const url = el.getAttribute('data-url');
const doRequest = async () => {
el.disabled = true;
await fetchActionDoRequest(el, url, {method: 'POST', headers: {'X-Csrf-Token': csrfToken}});
el.disabled = false;
};

const modalConfirmContent = htmlEscape($this.attr('data-modal-confirm') || '');
const modalConfirmContent = htmlEscape(el.getAttribute('data-modal-confirm') || '');
if (!modalConfirmContent) {
doRequest();
await doRequest();
return;
}

const isRisky = $this.hasClass('red') || $this.hasClass('yellow') || $this.hasClass('orange') || $this.hasClass('negative');
const isRisky = el.classList.contains('red') || el.classList.contains('yellow') || el.classList.contains('orange') || el.classList.contains('negative');
if (await confirmModal({content: modalConfirmContent, buttonColor: isRisky ? 'orange' : 'green'})) {
doRequest();
await doRequest();
}
}

Expand Down Expand Up @@ -354,7 +327,6 @@ export function initGlobalLinkActions() {

// Helpers.
$('.delete-button').on('click', showDeletePopup);
$('.link-action').on('click', linkAction);
}

function initGlobalShowModal() {
Expand Down