diff --git a/web/src/actions/build.js b/web/src/actions/build.js index 41a5b1b3b1..9b810e6aea 100644 --- a/web/src/actions/build.js +++ b/web/src/actions/build.js @@ -27,10 +27,12 @@ export const BUILDSET_FETCH_FAIL = 'BUILDSET_FETCH_FAIL' export const BUILD_OUTPUT_REQUEST = 'BUILD_OUTPUT_FETCH_REQUEST' export const BUILD_OUTPUT_SUCCESS = 'BUILD_OUTPUT_FETCH_SUCCESS' export const BUILD_OUTPUT_FAIL = 'BUILD_OUTPUT_FETCH_FAIL' +export const BUILD_OUTPUT_NOT_AVAILABLE = 'BUILD_OUTPUT_NOT_AVAILABLE' export const BUILD_MANIFEST_REQUEST = 'BUILD_MANIFEST_FETCH_REQUEST' export const BUILD_MANIFEST_SUCCESS = 'BUILD_MANIFEST_FETCH_SUCCESS' export const BUILD_MANIFEST_FAIL = 'BUILD_MANIFEST_FETCH_FAIL' +export const BUILD_MANIFEST_NOT_AVAILBLE = 'BUILD_MANIFEST_NOT_AVAILABLE' export const requestBuild = () => ({ type: BUILD_FETCH_REQUEST @@ -249,46 +251,81 @@ const failedBuildManifest = (error, url) => { } } -export const fetchBuild = (tenant, buildId, state, force) => dispatch => { - const build = state.build.builds[buildId] - if (!force && build) { - return Promise.resolve() - } - dispatch(requestBuild()) - return API.fetchBuild(tenant.apiPrefix, buildId) - .then(response => { - dispatch(receiveBuild(buildId, response.data)) - }) - .catch(error => dispatch(failedBuild(error, tenant.apiPrefix))) +function buildOutputNotAvailable() { + return { type: BUILD_OUTPUT_NOT_AVAILABLE } } -const fetchBuildOutput = (buildId, state, force) => dispatch => { - const build = state.build.builds[buildId] - const url = build.log_url.substr(0, build.log_url.lastIndexOf('/') + 1) - if (!force && build.output) { - return Promise.resolve() +function buildManifestNotAvailable() { + return { type: BUILD_MANIFEST_NOT_AVAILBLE } +} + +export function fetchBuild(tenant, buildId, state) { + return async function (dispatch) { + // Although it feels a little weird to not do anything in an action creator + // based on the redux state, we do this in here because the function is + // called from multiple places and it's easier to check for the build in + // here than in all the other places before calling this function. + if (state.build.builds[buildId]) { + return Promise.resolve() + } + + dispatch(requestBuild()) + try { + const response = await API.fetchBuild(tenant.apiPrefix, buildId) + dispatch(receiveBuild(buildId, response.data)) + } catch (error) { + dispatch(failedBuild(error, tenant.apiPrefix)) + // Raise the error again, so fetchBuildAllInfo() doesn't call the + // remaining fetch methods. + throw error + } } - dispatch(requestBuildOutput()) - return Axios.get(url + 'job-output.json.gz') - .then(response => dispatch(receiveBuildOutput(buildId, response.data))) - .catch(error => { +} + +function fetchBuildOutput(buildId, state) { + return async function (dispatch) { + // As this function is only called after fetchBuild() we can assume that + // the build is in the state. Otherwise an error would have been thrown and + // this function wouldn't be called. + const build = state.build.builds[buildId] + if (!build.log_url) { + // Don't treat a missing log URL as failure as we don't want to show a + // toast for that. The UI already informs about the missing log URL in + // multiple places. + dispatch(buildOutputNotAvailable()) + return Promise.resolve() + } + const url = build.log_url.substr(0, build.log_url.lastIndexOf('/') + 1) + dispatch(requestBuildOutput()) + try { + const response = await Axios.get(url + 'job-output.json.gz') + dispatch(receiveBuildOutput(buildId, response.data)) + } catch (error) { if (!error.request) { + dispatch(failedBuildOutput(error, url)) + // Raise the error again, so fetchBuildAllInfo() doesn't call the + // remaining fetch methods. throw error } - // Try without compression - Axios.get(url + 'job-output.json') - .then(response => dispatch(receiveBuildOutput( - buildId, response.data))) - }) - .catch(error => dispatch(failedBuildOutput(error, url))) + try { + // Try without compression + const response = await Axios.get(url + 'job-output.json') + dispatch(receiveBuildOutput(buildId, response.data)) + } catch (error) { + dispatch(failedBuildOutput(error, url)) + // Raise the error again, so fetchBuildAllInfo() doesn't call the + // remaining fetch methods. + throw error + } + } + } } -export const fetchBuildManifest = (buildId, state, force) => dispatch => { +export const fetchBuildManifest = (buildId, state) => (dispatch) => { + // As this function is only called after fetchBuild() we can assume that + // the build is in the state. Otherwise an error would have been thrown and + // this function wouldn't be called. const build = state.build.builds[buildId] - if (!force && build.manifest) { - return Promise.resolve() - } - dispatch(requestBuildManifest()) for (let artifact of build.artifacts) { if ('metadata' in artifact && @@ -301,15 +338,26 @@ export const fetchBuildManifest = (buildId, state, force) => dispatch => { .catch(error => dispatch(failedBuildManifest(error, artifact.url))) } } - dispatch(failedBuildManifest('no manifest found')) + // Don't treat a missing manifest file as failure as we don't want to show a + // toast for that. + dispatch(buildManifestNotAvailable()) } -export const fetchBuildIfNeeded = (tenant, buildId, force) => (dispatch, getState) => { - dispatch(fetchBuild(tenant, buildId, getState(), force)) - .then(() => { - dispatch(fetchBuildOutput(buildId, getState(), force)) - dispatch(fetchBuildManifest(buildId, getState(), force)) - }) +export function fetchBuildAllInfo(tenant, buildId) { + // This wraps the calls to fetch the build, output and manifest together as + // this is the common use case we have when loading the build info. + return async function (dispatch, getState) { + try { + // Wait for the build info to be available and provide the current status + // to the fetchBuildOutput and fetchBuildManifest so they can get the log + // url from the fetched build. + await dispatch(fetchBuild(tenant, buildId, getState())) + dispatch(fetchBuildOutput(buildId, getState())) + dispatch(fetchBuildManifest(buildId, getState())) + } catch (error) { + dispatch(failedBuild(error, tenant.apiPrefix)) + } + } } export const requestBuildset = () => ({ diff --git a/web/src/pages/Build.jsx b/web/src/pages/Build.jsx index 3ac98a4620..fcd9c2c953 100644 --- a/web/src/pages/Build.jsx +++ b/web/src/pages/Build.jsx @@ -36,7 +36,7 @@ import { PollIcon, } from '@patternfly/react-icons' -import { fetchBuildIfNeeded } from '../actions/build' +import { fetchBuildAllInfo } from '../actions/build' import { EmptyPage } from '../containers/Errors' import { Fetchable, Fetching } from '../containers/Fetching' import ArtifactList from '../containers/build/Artifact' @@ -48,22 +48,24 @@ import Manifest from '../containers/build/Manifest' class BuildPage extends React.Component { static propTypes = { match: PropTypes.object.isRequired, - remoteData: PropTypes.object, - tenant: PropTypes.object, - dispatch: PropTypes.func, + build: PropTypes.object, + isFetching: PropTypes.bool.isRequired, + isFetchingManifest: PropTypes.bool.isRequired, + isFetchingOutput: PropTypes.bool.isRequired, + tenant: PropTypes.object.isRequired, + fetchBuildAllInfo: PropTypes.func.isRequired, activeTab: PropTypes.string.isRequired, - location: PropTypes.object, - history: PropTypes.object, + location: PropTypes.object.isRequired, + history: PropTypes.object.isRequired, } - updateData = (force) => { - this.props.dispatch( - fetchBuildIfNeeded( + updateData = () => { + if (!this.props.build) { + this.props.fetchBuildAllInfo( this.props.tenant, - this.props.match.params.buildId, - force + this.props.match.params.buildId ) - ) + } } componentDidMount() { @@ -107,11 +109,18 @@ class BuildPage extends React.Component { } render() { - const { remoteData, activeTab, location, tenant } = this.props - const build = remoteData.builds[this.props.match.params.buildId] + const { + build, + isFetching, + isFetchingManifest, + isFetchingOutput, + activeTab, + location, + tenant, + } = this.props const hash = location.hash.substring(1).split('/') - if (!build && remoteData.isFetching) { + if (!build && isFetching) { return } @@ -127,14 +136,11 @@ class BuildPage extends React.Component { } const fetchable = ( - + ) const resultsTabContent = - !build.hosts && remoteData.isFetchingOutput ? ( + !build.hosts && isFetchingOutput ? ( ) : build.hosts ? ( @@ -159,7 +165,7 @@ class BuildPage extends React.Component { ) const logsTabContent = - !build.manifest && remoteData.isFetchingManifest ? ( + !build.manifest && isFetchingManifest ? ( ) : build.manifest ? ( @@ -173,7 +179,7 @@ class BuildPage extends React.Component { ) const consoleTabContent = - !build.output && remoteData.isFetchingOutput ? ( + !build.output && isFetchingOutput ? ( ) : build.output ? ( ({ - tenant: state.tenant, - remoteData: state.build, -}))(withRouter(BuildPage)) +function mapStateToProps(state, ownProps) { + const buildId = ownProps.match.params.buildId + const build = + buildId && Object.keys(state.build.builds).length > 0 + ? state.build.builds[buildId] + : null + return { + build, + tenant: state.tenant, + isFetching: state.build.isFetching, + isFetchingManifest: state.build.isFetchingManifest, + isFetchingOutput: state.build.isFetchingOutput, + } +} + +const mapDispatchToProps = { fetchBuildAllInfo } + +export default connect( + mapStateToProps, + mapDispatchToProps +)(withRouter(BuildPage))